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

fix(sct): Dont require Okta for commands which do not need it #10542

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pehala
Copy link
Contributor

@pehala pehala commented Apr 1, 2025

Testing

  • locally

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

Reminders

  • Add New configuration option and document them (in sdcm/sct_config.py)
  • Add unit tests to cover my changes (under unit-test/ folder)
  • Update the Readme/doc folder relevant to this change (if needed)

* update-conf-docs, nemesis-list, create-nemesis-pipelines
* This is a hotfix and it is ugly, but for 3 commands no bigger effort is needed
* Closes scylladb#10534
@pehala pehala added the backport/none Backport is not required label Apr 1, 2025
@fruch
Copy link
Contributor

fruch commented Apr 1, 2025

ugly is the new beautiful

doing better, is to completely regroup them differently, and that can't be done without change the command line, which I we don't want to do for this reason.

maybe we would be able to split and decorate each command with something, but that would also gonna be tricky and error prone (everyone adding a command would need to know all of the dependencies)

@fruch
Copy link
Contributor

fruch commented Apr 1, 2025

one more option, would be to move those commands out of sct.py, to their specific cli

which would be called like:

hydra python nemesi_cli.py ...

yet again, changing how the cli is gonna be called.

@pehala
Copy link
Contributor Author

pehala commented Apr 1, 2025

Yeah I agree, I also thought of some solutions, but none of them seemed to be worth it for this small change.

@fruch
Copy link
Contributor

fruch commented Apr 1, 2025

from new addition of command, we could consider separate modules

and that's looks interesting way add skip-able parts
https://stackoverflow.com/questions/50883972/click-how-do-i-apply-an-action-to-all-commands-and-subcommands-but-allow-a-comm

@scylladbbot
Copy link

@pehala new branch manager-3.5 was added, please add backport label if needed

@pehala
Copy link
Contributor Author

pehala commented Apr 3, 2025

from new addition of command, we could consider separate modules

and that's looks interesting way add skip-able parts https://stackoverflow.com/questions/50883972/click-how-do-i-apply-an-action-to-all-commands-and-subcommands-but-allow-a-comm

Looks interesting, but I still feel this is too big of a solution for 3 commands, for a part of code which does not see many changes. I would keep the current solution and if we need to touch commands again in a bigger way we can include it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/none Backport is not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants