Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #36575 - Use apipie-dsl default model descriptions #9867

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

archanaserver
Copy link
Contributor

@archanaserver archanaserver commented Oct 18, 2023

This pull request updates the apipie-dsl gem to version 2.6.1 and makes configuration changes to enhance our API documentation.

@archanaserver archanaserver requested a review from a team as a code owner October 18, 2023 11:01
@archanaserver archanaserver changed the title Fixes #36575- Update apipie-dsl gem Fixes #36575 - Update apipie-dsl gem Oct 18, 2023
@archanaserver archanaserver force-pushed the apipie-dsl-updates branch 2 times, most recently from 86592e3 to 0a58cde Compare October 18, 2023 11:06
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd squash this in a single commit, but in the same commit also remove all places where the model description is currently set. Otherwise the default will never be used. Then the summary of the commit would be to something like Use apipie-dsl default model descriptions.

config/initializers/apipie.rb Outdated Show resolved Hide resolved
@archanaserver archanaserver force-pushed the apipie-dsl-updates branch 2 times, most recently from 0449f58 to 46af197 Compare October 19, 2023 18:09
@ekohl ekohl changed the title Fixes #36575 - Update apipie-dsl gem Fixes #36575 - Use apipie-dsl default model descriptions Oct 19, 2023
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed roughly what I would expect. There are 2 more cases I see:

$ rg 'A class represent'
config/initializers/apipie.rb
30:  config.default_class_description = ->(model) { _("A class representing %s object") % model.human }

app/models/hostgroup.rb
86:  apipie :class, "A class representing #{model_name.human} object" do

app/models/subnet.rb
127:  apipie :class, "A class representing #{model_name.human} object" do

app/models/ptable.rb
64:  apipie :class, desc: 'A class representing Partition Table object' do

app/models/nic/base.rb
83:    apipie :class, 'A class representing Network Interface Controller object' do

app/models/host/base.rb
80:    apipie :class, 'A class representing base Host object' do

app/models/host/managed.rb
106:  apipie :class, 'A class representing managed Host object' do

So app/models/hostgroup.rb and app/models/subnet.rb are also candidates.

config/initializers/apipie.rb Outdated Show resolved Hide resolved
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failure looks related:

[2023-10-20T09:35:11.463Z] NoMethodError: undefined method `human' for User (call 'User.connection' to establish a connection):Class
[2023-10-20T09:35:11.463Z] /usr/local/rvm/gems/ruby-2.7.4@foreman-pr-unit-test-1661-2/gems/activerecord-6.1.7.6/lib/active_record/dynamic_matchers.rb:22:in `method_missing'

@archanaserver
Copy link
Contributor Author

Test failure looks related:

[2023-10-20T09:35:11.463Z] NoMethodError: undefined method `human' for User (call 'User.connection' to establish a connection):Class
[2023-10-20T09:35:11.463Z] /usr/local/rvm/gems/ruby-2.7.4@foreman-pr-unit-test-1661-2/gems/activerecord-6.1.7.6/lib/active_record/dynamic_matchers.rb:22:in `method_missing'

@ekohl I'm not sure where you're pointing it to, Can you please explain?

@ekohl
Copy link
Member

ekohl commented Oct 24, 2023

@ekohl I'm not sure where you're pointing it to, Can you please explain?

I opened the unit test results. The current one fails with another error (https://ci.theforeman.org/blue/organizations/jenkins/foreman-pr-unit-test/detail/foreman-pr-unit-test/1672/pipeline/) but I also see this in the Katello failure:

[2023-10-23T09:03:10.156Z] NotImplementedError: Katello::Model is an abstract class and cannot be instantiated.
[2023-10-23T09:03:10.156Z] /usr/local/rvm/gems/ruby-2.7.4@foreman-pr-katello-test-1695/gems/activerecord-6.1.7.6/lib/active_record/inheritance.rb:54:in `new'
[2023-10-23T09:03:10.156Z] /home/jenkins/workspace/foreman-pr-katello-test/foreman/config/initializers/apipie.rb:24:in `block (2 levels) in <top (required)>'
[2023-10-23T09:03:10.156Z] /usr/local/rvm/gems/ruby-2.7.4@foreman-pr-katello-test-1695/gems/apipie-dsl-2.6.0/lib/apipie_dsl/dsl.rb:469:in `apipie'
[2023-10-23T09:03:10.156Z] /home/jenkins/workspace/foreman-pr-katello-test/app/models/katello/model.rb:6:in `<class:Model>'
[2023-10-23T09:03:10.156Z] /home/jenkins/workspace/foreman-pr-katello-test/app/models/katello/model.rb:2:in `<module:Katello>'
[2023-10-23T09:03:10.156Z] /home/jenkins/workspace/foreman-pr-katello-test/app/models/katello/model.rb:1:in `<top (required)>'

@ekohl
Copy link
Member

ekohl commented Oct 26, 2023

And we're back to this error:

[2023-10-26T09:52:26.458Z] rake aborted!
[2023-10-26T09:52:26.458Z] NoMethodError: undefined method `human' for User (call 'User.connection' to establish a connection):Class

@ofedoren I think that it calls the descriptions during the initializer is a problem and a blocker to using this feature. Do you agree?

@archanaserver perhaps you can look in apipie-dsl itself to see if this can be fixed.

@archanaserver
Copy link
Contributor Author

And we're back to this error:

[2023-10-26T09:52:26.458Z] rake aborted!
[2023-10-26T09:52:26.458Z] NoMethodError: undefined method `human' for User (call 'User.connection' to establish a connection):Class

@ofedoren I think that it calls the descriptions during the initializer is a problem and a blocker to using this feature. Do you agree?

@archanaserver perhaps you can look in apipie-dsl itself to see if this can be fixed.

@ekohl @ofedoren I didn't find anything related to this issue. Could you please point me in the right direction? I'll take another look.

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've released apipie-dsl 2.6.1 with a fix to postpone default description block evaluation until the actual description renders. It should help for most of the time.

Although, there should be a slight change in config here. Please see inline.

@@ -27,6 +27,8 @@
trans
end
config.help_layout = 'apipie_dsl/apipie_dsls/help.html.erb'
config.default_class_description = ->(model) { _("A class representing %s object") % model.human }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
config.default_class_description = ->(model) { _("A class representing %s object") % model.human }
config.default_class_description = ->(model) do
return nil unless model.respond_to?(:model_name)
_("A class representing %s object") % model.model_name.human
end

It appears that model doesn't necessary respond to human, but rather should respond (in case it's ActiveRecord) to model_name, which returns some wrapper, which responds to human.

Gemfile Outdated
@@ -11,7 +11,7 @@ gem 'ancestry', '~> 4.0'
gem 'scoped_search', '>= 4.1.10', '< 5'
gem 'ldap_fluff', '>= 0.5.0', '< 1.0'
gem 'apipie-rails', '>= 0.8.0', '< 2'
gem 'apipie-dsl', '>= 2.2.6'
gem 'apipie-dsl', '>= 2.6.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should at least require 2.6.1

Suggested change
gem 'apipie-dsl', '>= 2.6.0'
gem 'apipie-dsl', '>= 2.6.1'

@@ -27,6 +27,11 @@
trans
end
config.help_layout = 'apipie_dsl/apipie_dsls/help.html.erb'
config.default_class_description = ->(model) do
Copy link
Member

@ekohl ekohl Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think RuboCop wants to use lambda:

Suggested change
config.default_class_description = ->(model) do
config.default_class_description = lambda do |model|

See https://docs.rubocop.org/rubocop/1.57/cops_style.html#stylelambda as well

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Packit fails to build because it wasn't rebased, but that's not blocking here.

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to add from my side, thanks a lot! Let's get this in.

@ofedoren ofedoren merged commit 65f2a01 into theforeman:develop Nov 10, 2023
8 of 9 checks passed
@archanaserver archanaserver deleted the apipie-dsl-updates branch November 15, 2023 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants