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

Add support for LEAPP data downloads #750

Closed

Conversation

ShimShtein
Copy link
Member

LEAPP upgrade tool needs to download files from the cloud, so they need support for forwarding their requests through the server similar to insights-client.

@ehelms
Copy link
Member

ehelms commented Jul 27, 2022

Thinking a bit about this from a dependency stand point, I tend to expect either:

a) this to live in foreman_leapp with a dependency on foreman_rh_cloud
b) foreman_rh_cloud to have a dependency on, or at least check that foreman_leapp is present to enable this set of functionality
c) foreman_leapp to have a dependency on foreman_rh_cloud

It won't be obvious in the current form that there is some leapp related functionality over here in foreman_rh_cloud when trying to debug this or understand the workflow.

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.

a) this to live in foreman_leapp with a dependency on foreman_rh_cloud

foreman_leapp lives in plugins, foreman_rh_cloud lives in and depends on katello. You can't have that dependency. Or it needs to be a soft dependency where foreman_leapp detects the presence of foreman_rh_cloud at runtime and enables functionality.

c) foreman_leapp to have a dependency on foreman_rh_cloud

For the same reason, this will break repoclosure.

@@ -99,6 +105,10 @@ def connection_test_request?
->(request_path) { request_path =~ /redhat_access\/r\/insights\/?$/ }
end

def pes_request?
->(request_path) { request_path =~ /api\/pes\// }
Copy link
Member

Choose a reason for hiding this comment

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

I see it was already there before, but have you considered the %r{} notation?

Suggested change
->(request_path) { request_path =~ /api\/pes\// }
->(request_path) { request_path =~ %r{api/pes/} }

Also note you only perform partial matching, so /something/elseapi/pes/ will also match.

Copy link
Member Author

Choose a reason for hiding this comment

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

It comes with the routes filter, so something/else/api/pes wouldn't get here anyway.

@evgeni
Copy link
Member

evgeni commented Aug 22, 2022

I don't think foreman_leapp should be a trigger.

People can use Leapp w/o foreman_leapp on systems that are behind a Katello.

@ShimShtein ShimShtein marked this pull request as draft September 7, 2022 07:37
@ShimShtein
Copy link
Member Author

Converting this to draft, as the team asked us to wait with this feature

@ShimShtein
Copy link
Member Author

Closing, since it is not needed for too long

@ShimShtein ShimShtein closed this May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants