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.

Sign up for GitHub

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

Jump to bottom

Fixes #36083 - Remove remote_execution_by_default setting #10688

Merged
merged 1 commit into from
Aug 17, 2023
Merged

Fixes #36083 - Remove remote_execution_by_default setting #10688

merged 1 commit into from
Aug 17, 2023

Conversation

jeremylenz
Copy link
Member

What are the changes introduced in this pull request?

  • Remove the setting 'Use remote execution by default'
  • Everywhere in the code that used it, change behavior to use remote execution no matter what

Considerations taken when implementing this change?

Should probably wait to merge this one until any UI changes that depend on it are merged.

What are the testing steps for this pull request?

  • Test incremental updates via REX and make sure nothing is broken
  • Test legacy content host UI and ensure it always uses REX and nothing is broken
  • Test new host details page and ensure it always uses REX and nothing is broken
  • on new host details page > Content > Packages > kebab > Install packages, ensure 'via Katello agent' option is NOT listed

@theforeman-bot
Copy link

Issues: #36083

lfu
lfu reviewed Aug 15, 2023
View reviewed changes
lib/katello/engine.rb Outdated Show resolved Hide resolved
lfu
lfu reviewed Aug 15, 2023
View reviewed changes
app/lib/actions/katello/content_view/incremental_updates.rb
def plan(version_environments, composite_version_environments, content, dep_solve, hosts, description,
use_remote_execution = false)
def plan(version_environments, composite_version_environments, content, dep_solve, hosts, description) # rubocop:disable Metrics/MethodLength
use_remote_execution = true # TODO: remove this when we remove katello-agent dynflow actions
Copy link
Member

Choose a reason for hiding this comment

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

This makes code on line 38-40 would never run.
Do we really need it?

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 didn't want to get too deep into editing Dynflow actions here, since Dynflow actions will be in another PR.

@jeremylenz jeremylenz force-pushed the 36083-remove-katello-agent-setting branch from 4993bb5 to 4339741 Compare August 16, 2023 20:10
@jeremylenz
Copy link
Member Author

@lfu Removed remote_execution_by_default? method and the single reference to it.

lfu
lfu reviewed Aug 17, 2023
View reviewed changes
engines/bastion_katello/lib/bastion_katello/engine.rb
@@ -43,7 +43,6 @@ class Engine < ::Rails::Engine
'katelloAgentPresent' => ::Katello.with_katello_agent?,
'remoteExecutionPresent' => ::Katello.with_remote_execution?,
'hostToolingEnabled' => (::Katello.with_katello_agent? || ::Katello.with_remote_execution?) ? true : false,
'remoteExecutionByDefault' => ::Katello.remote_execution_by_default?
Copy link
Member

Choose a reason for hiding this comment

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

remoteExecutionByDefault is used by angular code. Maybe this line should be removed by the PR that cleans up angular code.

For this one, we can keep it as:

'remoteExecutionByDefault' => true

Copy link
Member Author

Choose a reason for hiding this comment

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

@chris1984 If you can move this change to your PR, I'll remove it here.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, If I remove this line but keep the method removed, it will break all the tests on this PR. @lfu I plan to merge the Angular PR first. I'm thinking it may be easier to just keep this PR the way it is.. @chris1984

Copy link
Member

Choose a reason for hiding this comment

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

Lol that's fine, I'll take it out of mine

@jeremylenz jeremylenz force-pushed the 36083-remove-katello-agent-setting branch from 4339741 to d7b0fc0 Compare August 17, 2023 14:58
@jeremylenz
Copy link
Member Author

fixed rubocop

lfu
lfu reviewed Aug 17, 2023
View reviewed changes
engines/bastion_katello/lib/bastion_katello/engine.rb
@@ -42,8 +42,7 @@ class Engine < ::Rails::Engine
'defaultDownloadPolicy' => !Foreman.in_rake? && db_migrated && Setting['default_download_policy'],
'katelloAgentPresent' => ::Katello.with_katello_agent?,
'remoteExecutionPresent' => ::Katello.with_remote_execution?,
'hostToolingEnabled' => (::Katello.with_katello_agent? || ::Katello.with_remote_execution?) ? true : false,
'remoteExecutionByDefault' => ::Katello.remote_execution_by_default?
'hostToolingEnabled' => (::Katello.with_katello_agent? || ::Katello.with_remote_execution?) ? true : false
Copy link
Member

Choose a reason for hiding this comment

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

This line should be taken care of as well by some PR.

@lfu
Copy link
Member

lfu commented Aug 17, 2023

Tests went well with hammer incremental update.
Content - Errata - apply worked.
Nothing was broken in Content Host and new Hosts detail pages for REX installation.

lfu
lfu approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@lfu lfu left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@parthaa parthaa merged commit 283c63d into Katello:master Aug 17, 2023
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris1984 chris1984 chris1984 left review comments

@lfu lfu lfu approved these changes

Assignees
No one assigned
Labels
Not yet reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jeremylenz @theforeman-bot @lfu @chris1984 @parthaa