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 #31278 - include lib directory in production #516

Closed
wants to merge 1 commit into from

Conversation

Ron-Lavi
Copy link
Member

@Ron-Lavi Ron-Lavi commented Nov 9, 2020

No description provided.

@theforeman-bot
Copy link
Member

Issues: #31278

@Ron-Lavi
Copy link
Member Author

Ron-Lavi commented Nov 9, 2020

@lzap @rabajaj0509 can you take a look please?

in nightly I got the following error: uninitialized constant ForemanDiscovery::VERSION
which we are trying to pass into the EmptyState component in

docUrl: "https://theforeman.org/plugins/foreman_discovery/#{ForemanDiscovery::VERSION.scan(/\d+\.\d+/).first}/index.html" ) %>

I added the lib dir to be loaded with eager_load, and now the component is visible and works as expected

@rabajaj0509
Copy link
Member

@LaViro waiting for tests to pass, once they pass, I shall merge this PR :) good job here buddy!

@lzap
Copy link
Member

lzap commented Nov 9, 2020

I don't understand the failure ActiveRecord::AdapterNotSpecified: The productiondatabase is not configured for theproduction environment. That is probably not related?

Can you maybe give little bit more context about the change? Doesn't this break auto-loading in that directory? I can't comment much as Rails non-expert about this one. Maybe @ezr-ondrej or @tbrisker can help to verify if eager_load like that is correct?

@lzap
Copy link
Member

lzap commented Nov 9, 2020

[test discovery]

@Ron-Lavi
Copy link
Member Author

Ron-Lavi commented Nov 9, 2020

@Ron-Lavi
Copy link
Member Author

Ron-Lavi commented Nov 9, 2020

sorry for the last ugly push, didn't realize discovery has both master and develop branches :D

@lzap
Copy link
Member

lzap commented Nov 9, 2020

Whoops, okay. Still running...

@lzap
Copy link
Member

lzap commented Nov 9, 2020

Again the same error, is this something expected, maybe 2.7 Ruby is too new on Jenkins? @ekohl or @tbrisker ? I scroll all the output and I could not find any other error just the database production one from above.

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.

in nightly I got the following error: uninitialized constant ForemanDiscovery::VERSION

I think the Foreman plugin already has a version via Gems after you call register.

https://github.com/theforeman/foreman/blob/a600617c1ce75582fdb61a7dca5d549fc6a01cca/app/registries/foreman/plugin.rb#L102

That means it'd be better to avoid the constant and get the plugin instance. Then you can call version on it:

Foreman::Plugin.find(:foreman_discovery).version

Untested, but I believe that to a cleaner fix.

Also, it may break branding if you do it this way. A better way would be to use the LinksController. However, it looks like it's currently not possible to pass the version (https://github.com/theforeman/foreman/blob/a600617c1ce75582fdb61a7dca5d549fc6a01cca/app/controllers/links_controller.rb#L13-L14) even though its implementation supports this (https://github.com/theforeman/foreman/blob/a600617c1ce75582fdb61a7dca5d549fc6a01cca/app/controllers/links_controller.rb#L37-L41). You should submit a PR to support that and then use it.

Again the same error, is this something expected, maybe 2.7 Ruby is too new on Jenkins? @ekohl or @tbrisker ? I scroll all the output and I could not find any other error just the database production one from above.

I wouldn't expect the Ruby 2.7 version to time out after 2 hours. Perhaps it the loading is entering some sort of loop?

@@ -15,6 +15,8 @@ class Engine < ::Rails::Engine
config.autoload_paths += Dir["#{config.root}/app/models/concerns"]
config.autoload_paths += Dir["#{config.root}/app/services"]

config.eager_load_paths += Dir["#{config.root}/lib"]
Copy link
Member

Choose a reason for hiding this comment

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

This feels very weird. This file itself is in lib so isn't it already loaded at this point?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not very familiar with how foreman loads the plugins, maybe it is looking for a specific engine file? And the rest of the lib is ignored?

I will try tomorrow the solution you suggested in nightly, meanwhile adding @ShimShtein to the party :)

@Ron-Lavi
Copy link
Member Author

Doing Foreman::Plugin.find(:foreman_discovery).version made that page to work, thanks @ekohl !!
I will open another PR

@lzap
Copy link
Member

lzap commented Nov 10, 2020

Thanks for help with this one @ekohl ! I am gonna close this one, the other one looks much cleaner.

@lzap lzap closed this Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants