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

Additional arguments in foreman-maintain #2244

Closed

Conversation

mjivraja
Copy link
Contributor

The additional arguments that can be used for foreman-maintain can be listed while updating server to next minor version.

https://bugzilla.redhat.com/show_bug.cgi?id=2189967

Please cherry-pick my commits into:

  • Foreman 3.7/Katello 4.9 (planned Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13)
  • Foreman 3.4/Katello 4.6 (EL8 only)
  • Foreman 3.3/Katello 4.5 on EL7 & EL8 (Satellite 6.12 on EL8 only; orcharhino 6.4 on EL8 only)
  • Foreman 3.2/Katello 4.4 on EL7 & EL8
  • Foreman 3.1/Katello 4.3 on EL7 & EL8 (Satellite 6.11 EL7/8; orcharhino 6.3 on EL7/8)
  • For Foreman 3.0 or older, please create a separate PR.
  • We do not accept PRs for Foreman 2.4 or older.

The additional arguments that can be used for foreman-maintain can be
listed while updating server to next minor version.

https://bugzilla.redhat.com/show_bug.cgi?id=2189967
@github-actions
Copy link

The PR preview for bb635a3 is available at theforeman-foreman-documentation-preview-pr-2244.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

@mjivraja
Copy link
Contributor Author

The merged changes of this PR shall be applicable to this PR #2233 as well.

@adamlazik1
Copy link
Contributor

Should it be explained what whitelist does and what whitelist arguments can be?

@mjivraja
Copy link
Contributor Author

Should it be explained what whitelist does and what whitelist arguments can be?

IMO, users may be aware of. But, let's wait for other reviews.

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

I am against recommending users to bypass certain upgrade checks; it is potentially dangerous if users just reword "check" to "run" to perform the upgrade.

But we're currently not using the procedure in downstream. If someone ACKs this, then it's good to go.

@ekohl
Copy link
Member

ekohl commented Jun 21, 2023

Like the others I doubt this is a good direction. A few reasons:

  • whitelist is poorly named: it's really a blacklist where it doesn't run checks that were listed (Change --whitelist to --skip foreman_maintain#723 renames it to --skip)
  • We don't want users to skip checks.
  • We don't provide any help on how to find out which checks the user could list

So overall I'd say this is not a direction we should head into. At best you could add a section on how to skip certain checks but it should not be the default user instruction.

@mjivraja
Copy link
Contributor Author

Like the others I doubt this is a good direction. A few reasons:

  • whitelist is poorly named: it's really a blacklist where it doesn't run checks that were listed (Change --whitelist to --skip foreman_maintain#723 renames it to --skip)
  • We don't want users to skip checks.
  • We don't provide any help on how to find out which checks the user could list

So overall I'd say this is not a direction we should head into. At best you could add a section on how to skip certain checks but it should not be the default user instruction.

Agree with this. Even prior collaborations with SMEs had the same point to direct the users in other ways, instead of whitelisting the parameters.

As it is not recommended to add the argument to this, I am closing this PR. Adding a note on my side of having some resources for users on using arguments.

@mjivraja mjivraja closed this Jun 21, 2023
@mjivraja mjivraja deleted the 17316-Whitelist-arguments-master branch June 22, 2023 12:07
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.

4 participants