Fix apidoc position of Puppet plugin #151

nadjaheitmann · 2021-05-21T12:37:03Z

This fixes #147

ezr-ondrej · 2021-05-23T12:40:49Z

app/controllers/foreman_puppet/api/v2/base_controller.rb

@@ -3,7 +3,8 @@ module Api
    module V2
      class BaseController < ::Api::V2::BaseController
        resource_description do
-          api_version '2'
+          resource_id 'foreman_puppet_base'


Where is the id of the resource used?

It is used by Apipie to distinguish between the classes and get the documentation correctly, as far as I can tell.

I should change the resource_id of the BaseController and the one of the LookupsCommonController to have a pp_ prefix such that all of the resource_ids are consistent.

nadjaheitmann · 2021-05-28T08:45:53Z

With this PR, the API entries appear in addition to the core entries. Is this okay or should they replace the core entries?

timogoebel · 2021-05-28T09:01:33Z

With this PR, the API entries appear in addition to the core entries. Is this okay or should they replace the core entries?

I believe when the plugin is installed, the core models and controllers don't work anymore and the endpoints from this plugin should be used.

nadjaheitmann · 2021-05-28T12:10:19Z

I believe when the plugin is installed, the core models and controllers don't work anymore and the endpoints from this plugin should be used.

As of now, @ezr-ondrej said that the old routes are redirected to the new ones. But in any case, I think it makes more sense to remove the old apidoc entries or at least mark them as deprecated as it gives the impression that both can be used in parallel. I will fix that.

timogoebel · 2021-05-31T13:19:48Z

I'm wondering if we can add something to the CI to verify all relevant fields are properly documented.

nadjaheitmann · 2021-06-01T06:32:02Z

I'm wondering if we can add something to the CI to verify all relevant fields are properly documented.

This would be very helpful indeed as comparing all API entries manually is very tedious. I will think about it. Let me know if you have an idea how to do it.

nadjaheitmann · 2021-06-01T06:48:10Z

As of now, @ezr-ondrej said that the old routes are redirected to the new ones.

Correction: Old routes are not redirected, yet, but end up at the core API. Do we want to fix this?

[I|app|e93963d1] Started GET "/api/puppetclasses" for 192.168.121.1 at 2021-06-01 06:45:45 +0000$
[I|app|e93963d1] Processing by Api::V2::PuppetclassesController#index as JSON

With this PR, the documentation of the core API is overwritten, such that only new routes are shown. Moreover, resource_id stays the same as in core for all commands, such that e.g. Hammer can automatically use the new routes instead of the old ones.

The PR fixes #147 that were caused mostly by resource_id inconsistencies between Foreman core and the plugin.

timogoebel · 2021-06-01T06:57:17Z

Correction: Old routes are not redirected, yet, but end up at the core API. Do we want to fix this?

Yes, I think we should. We could also just disable the core routes. I think we should just make sure this is actually worth the effort. I can see a lot of tests in core break if we implement this.

Let me know if you have an idea how to do it.

I think it should be fairly easy to get all relevant API controllers (e.g. via the routes) and all the actions. We then need possible parameters that we could get from the parameter filters. And to finish it up, the documentation dsl should give us a list of all already documented parameters.

nadjaheitmann · 2021-06-01T07:36:14Z

Yes, I think we should. We could also just disable the core routes. I think we should just make sure this is actually worth the effort. I can see a lot of tests in core break if we implement this.

I will open a new issue for this.

I think it should be fairly easy to get all relevant API controllers (e.g. via the routes) and all the actions. We then need possible parameters that we could get from the parameter filters. And to finish it up, the documentation dsl should give us a list of all already documented parameters.

Let me give it a try. I guess it makes sense to have the tests as part of this PR? But I have not seen tests for apidoc completion before, so could be a new issue as well...

timogoebel · 2021-06-01T07:41:12Z

But I have not seen tests for apidoc completion before, so could be a new issue as well...

I'd recommend a separate issue.

nadjaheitmann · 2021-06-08T08:56:55Z

Is there anything that needs fixing for this PR?

timogoebel

Looks good to me.

kamils-iRonin · 2021-06-09T07:42:49Z

app/controllers/foreman_puppet/api/v2/puppet_base_controller.rb

@@ -1,10 +1,9 @@
 module ForemanPuppet
  module Api
    module V2
-      class BaseController < ::Api::V2::BaseController
+      class PuppetBaseController < ::Api::V2::BaseController


Is there any other reason for renaming the controllers, or do you just feel it's a better name?

The reason is that the resource_id of any controller is generated from the name of the controller. As there already exist a controller named BaseController in Foreman core, there is a name clash. It is okay for the controllers that should be overwritten anyways, but not for the ones that should remain in Foreman core.

kamils-iRonin · 2021-06-09T07:58:52Z

Thanks @nadjaheitmann 👍

nadjaheitmann force-pushed the fix_apidoc branch 2 times, most recently from 4c83e3f to e5f8353 Compare May 21, 2021 12:39

nadjaheitmann changed the title ~~Fix apidoc~~ WIP: Fix apidoc May 21, 2021

nadjaheitmann force-pushed the fix_apidoc branch from e5f8353 to 26c0e3a Compare May 21, 2021 12:45

nadjaheitmann changed the title ~~WIP: Fix apidoc~~ Fix apidoc May 21, 2021

nadjaheitmann changed the title ~~Fix apidoc~~ Fix apidoc position of Puppet plugin May 21, 2021

ezr-ondrej reviewed May 23, 2021

View reviewed changes

nadjaheitmann changed the title ~~Fix apidoc position of Puppet plugin~~ WIP: Fix apidoc position of Puppet plugin May 28, 2021

nadjaheitmann force-pushed the fix_apidoc branch from 26c0e3a to 77f5623 Compare May 31, 2021 12:20

Fix apidoc

22bf5c4

nadjaheitmann force-pushed the fix_apidoc branch from 77f5623 to 22bf5c4 Compare May 31, 2021 12:34

Fix tests for new PuppetLookupsCommonController class

cec68b7

This was referenced Jun 1, 2021

Redirect core routes to use plugin routes #152

Closed

Add tests for apidoc #153

Closed

nadjaheitmann changed the title ~~WIP: Fix apidoc position of Puppet plugin~~ Fix apidoc position of Puppet plugin Jun 1, 2021

timogoebel approved these changes Jun 9, 2021

View reviewed changes

kamils-iRonin reviewed Jun 9, 2021

View reviewed changes

kamils-iRonin approved these changes Jun 9, 2021

View reviewed changes

kamils-iRonin merged commit 1737600 into theforeman:master Jun 9, 2021

nadjaheitmann deleted the fix_apidoc branch June 9, 2021 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix apidoc position of Puppet plugin #151

Fix apidoc position of Puppet plugin #151

nadjaheitmann commented May 21, 2021 •

edited

Loading

ezr-ondrej May 23, 2021

nadjaheitmann May 25, 2021

nadjaheitmann May 26, 2021

nadjaheitmann commented May 28, 2021

timogoebel commented May 28, 2021 •

edited

Loading

nadjaheitmann commented May 28, 2021 •

edited

Loading

timogoebel commented May 31, 2021

nadjaheitmann commented Jun 1, 2021

nadjaheitmann commented Jun 1, 2021 •

edited

Loading

timogoebel commented Jun 1, 2021

nadjaheitmann commented Jun 1, 2021

timogoebel commented Jun 1, 2021

nadjaheitmann commented Jun 8, 2021

timogoebel left a comment

kamils-iRonin Jun 9, 2021

nadjaheitmann Jun 9, 2021

kamils-iRonin commented Jun 9, 2021

Fix apidoc position of Puppet plugin #151

Fix apidoc position of Puppet plugin #151

Conversation

nadjaheitmann commented May 21, 2021 • edited Loading

ezr-ondrej May 23, 2021

Choose a reason for hiding this comment

nadjaheitmann May 25, 2021

Choose a reason for hiding this comment

nadjaheitmann May 26, 2021

Choose a reason for hiding this comment

nadjaheitmann commented May 28, 2021

timogoebel commented May 28, 2021 • edited Loading

nadjaheitmann commented May 28, 2021 • edited Loading

timogoebel commented May 31, 2021

nadjaheitmann commented Jun 1, 2021

nadjaheitmann commented Jun 1, 2021 • edited Loading

timogoebel commented Jun 1, 2021

nadjaheitmann commented Jun 1, 2021

timogoebel commented Jun 1, 2021

nadjaheitmann commented Jun 8, 2021

timogoebel left a comment

Choose a reason for hiding this comment

kamils-iRonin Jun 9, 2021

Choose a reason for hiding this comment

nadjaheitmann Jun 9, 2021

Choose a reason for hiding this comment

kamils-iRonin commented Jun 9, 2021

nadjaheitmann commented May 21, 2021 •

edited

Loading

timogoebel commented May 28, 2021 •

edited

Loading

nadjaheitmann commented May 28, 2021 •

edited

Loading

nadjaheitmann commented Jun 1, 2021 •

edited

Loading