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 #30094 - drop unwanted interactive options from packages cmd #490

Merged
merged 1 commit into from Jun 30, 2021

Conversation

upadhyeammit
Copy link
Contributor

The package sub-commands by default use to have 'assumeyes' and
'whitelist' options. These options are not required for all
sub-commands.

The packages subcommands by default use to have assumeyes and
whitelist options. These options are not required for all
subcommands.
@theforeman-bot
Copy link
Member

Issues: #30094

@@ -51,7 +51,7 @@ def execute
end

subcommand 'is-locked', 'Check if update of packages is allowed' do
interactive_option
interactive_option([])
Copy link
Member

Choose a reason for hiding this comment

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

The installer uses this:
https://github.com/theforeman/foreman-installer/blob/bc575c2d662dad4779623b89d24ca7750dcec3bd/hooks/pre_commit/09-version_locking.rb#L2

I'd say we should patch the installer (theforeman/foreman-installer#701) but considering downstream shipping perhaps this should also accept assumeyes?

Copy link

Choose a reason for hiding this comment

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

for the record - the issue linked to this PR also expects the assumeyes to be available for this subcommand (see "expected result")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekohl technically assumeyes is useless for is-locked command as there isn't any user interaction happening in the process. But if we drop the --assumeyes from the foremain-installer then it will just increase the one more if else block in maintain because there will be older versions of foreman-installer which are still using the --assumeyes. Hence I propose not to remove --assumeyes from the installer, and I readd it for the is-locked.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't --assumeyes also useless in older versions? So the installer could safely drop it without creating an else clause? The behavior should be the same, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes its useless for older versions. If you can cherry pick the installer changes in older versions too then we can keep this as is and change will only happen to installer.

@upadhyeammit upadhyeammit deleted the 30094 branch December 3, 2021 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants