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 #34706 - introduce an option to exclude step from whitelist #601

Merged
merged 2 commits into from
Apr 29, 2022

Conversation

upadhyeammit
Copy link
Contributor

No description provided.

@theforeman-bot
Copy link
Member

Issues: #34706

@evgeni
Copy link
Member

evgeni commented Apr 1, 2022

This doesn't prevent users from whitelisting those steps, right? It just doesn't suggest that it's possible/desireable to do so?

If the above is correct, then the patch should do what you say, but the commit message is misleading ;-)

@jlsherrill
Copy link
Contributor

tested this and this is what i got.

Tasks: TOP => katello:pulp3_content_switchover
(See full trace by running task with --trace)
Checking for valid Katello configuraton.
--------------------------------------------------------------------------------
Scenario [Migration scripts to Satellite 6.10] failed.

The following steps ended up in failing state:

  [content-switchover]

I kinda think we should make this step never skippable. Its asking for too many problems, but i'm fine with whatever support says

@upadhyeammit
Copy link
Contributor Author

This doesn't prevent users from whitelisting those steps, right? It just doesn't suggest that it's possible/desireable to do so?

If the above is correct, then the patch should do what you say, but the commit message is misleading ;-)

Yeah, I feel there could be still a situation where failures are false positives. In that case users should have option to skip it? Unless we are very sure whatever problems reported by the switchover are always legit.

@evgeni
Copy link
Member

evgeni commented Apr 1, 2022

I think that there are situations/steps that should not be disable-able, at all.

@upadhyeammit
Copy link
Contributor Author

This doesn't prevent users from whitelisting those steps, right? It just doesn't suggest that it's possible/desireable to do so?
If the above is correct, then the patch should do what you say, but the commit message is misleading ;-)

Yeah, I feel there could be still a situation where failures are false positives. In that case users should have option to skip it? Unless we are very sure whatever problems reported by the switchover are always legit.

tested this and this is what i got.

Tasks: TOP => katello:pulp3_content_switchover
(See full trace by running task with --trace)
Checking for valid Katello configuraton.
--------------------------------------------------------------------------------
Scenario [Migration scripts to Satellite 6.10] failed.

The following steps ended up in failing state:

  [content-switchover]

I kinda think we should make this step never skippable. Its asking for too many problems, but i'm fine with whatever support says

I like the idea however we need to be 100% sure that whatever failures reported by the content-switchover are always legit. This means there are no situation where we will use whitelist, which I think difficult to agree on?

@jlsherrill
Copy link
Contributor

The switchover step is so vital to be run successfully that i don't think it should be skippable. If a user skips it, they are likely completely destroying all the content views on their satellite and will have to start over from scratch. I'd rather have 100 users debug this failing and work through it than 1 user skipping it and losing everything.

@ekohl
Copy link
Member

ekohl commented Apr 6, 2022

I think that there are situations/steps that should not be disable-able, at all.

Should his be called "mandatory" instead?

@upadhyeammit
Copy link
Contributor Author

As per the discussion with Ashish I have modified the message for downstream situation. As example,

The following steps ended up in failing state:

  [pulp-remove]

Resolve the failed steps and rerun
the command.

If the situation persists and, you are unclear what to do next, 
contact Red Hat Technical Support. 

In case the failures are false positives,
use --whitelist="pulp-remove"

@upadhyeammit
Copy link
Contributor Author

@evgeni added the tests, if you think those can be written in some better way then suggestions are welcome :)

test/lib/reporter_test.rb Outdated Show resolved Hide resolved
@upadhyeammit upadhyeammit changed the title Fixes #34706 - introduce an option to mark step non whitelistable Fixes #34706 - introduce an option to exclude step from whitelist Apr 28, 2022
test/lib/reporter_test.rb Outdated Show resolved Hide resolved
test/lib/reporter_test.rb Outdated Show resolved Hide resolved
test/lib/reporter_test.rb Outdated Show resolved Hide resolved
@evgeni
Copy link
Member

evgeni commented Apr 28, 2022

Can we add a failing step to the tests, that has do_not_whitelist=true, so we can assert that it's not listed in the "you can whitelist things" output?

I think that's the last missing piece before I say ACK ;-)

@upadhyeammit
Copy link
Contributor Author

Can we add a failing step to the tests, that has do_not_whitelist=true, so we can assert that it's not listed in the "you can whitelist things" output?

I think that's the last missing piece before I say ACK ;-)

done :)

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

ACK!

@upadhyeammit upadhyeammit merged commit 72adb77 into theforeman:master Apr 29, 2022
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.

5 participants