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

Move the --user and --password arguments in a group with --openid-api #3550

Merged
merged 2 commits into from Oct 21, 2019

Conversation

@pypingou
Copy link
Member

pypingou commented Oct 10, 2019

This way action which allow to set the --openid-api arguments will not
forget to add the --user and --password ones (as did the waive and
request-tests).
This commit also fixes the waive and request-tests action so they allow
logging in from the CLI via the --user and --password arguments as do
other actions requiring to be logged in.

Signed-off-by: Pierre-Yves Chibon pingou@pingoured.fr

@pypingou pypingou requested a review from fedora-infra/bodhi as a code owner Oct 10, 2019
@pypingou pypingou force-pushed the pypingou:user_password_argument branch from 38559d6 to 765540c Oct 11, 2019
Copy link
Member

nphilipp left a comment

Would it make sense to add openid_options to new_edit_options, save_edit_options and release_options instead of adding the @add_options(openid_options) decorator to every function that uses these three?

@nphilipp nphilipp self-assigned this Oct 14, 2019
@pypingou

This comment has been minimized.

Copy link
Member Author

pypingou commented Oct 15, 2019

Would it make sense to add openid_options to new_edit_options, save_edit_options and release_options instead of adding the @add_options(openid_options) decorator to every function that uses these three?

new_edit_options, save_edit_options and release_options are all lists, so we could expend the lists where they are defined but then we end up with some method which are explicit about including the openid_options and some which aren't because they bundle these options in others (new_edit_options...).

@Zlopez
Zlopez approved these changes Oct 15, 2019
Copy link
Contributor

Zlopez left a comment

This looks much better

@nphilipp

This comment has been minimized.

Copy link
Member

nphilipp commented Oct 15, 2019

Would it make sense to add openid_options to new_edit_options, save_edit_options and release_options instead of adding the @add_options(openid_options) decorator to every function that uses these three?

new_edit_options, save_edit_options and release_options are all lists, so we could expend the lists where they are defined but then we end up with some method which are explicit about including the openid_options and some which aren't because they bundle these options in others (new_edit_options...).

Yeah, this could be more confusing than helpful.

@nphilipp

This comment has been minimized.

Copy link
Member

nphilipp commented Oct 15, 2019

@pypingou I'm not sure why the integration tests fail on f31 and Rawhide, they succeed in my Vagrant box. I'll force-push the branch without changes to retrigger the tests.

@nphilipp nphilipp force-pushed the pypingou:user_password_argument branch from e31f80e to 79974fb Oct 15, 2019
Copy link
Member

abompard left a comment

Do you think that this change should be listed in the next release notes? If so, please add a file in the news directory as explained in this documentation. Thanks!

Copy link
Member

abompard left a comment

I think this is worthy of an entry in the release notes because new options have been added to the CLI.

This way action which allow to set the --openid-api arguments will not
forget to add the --user and --password ones (as did the waive and
request-tests).
This commit also fixes the waive and request-tests action so they allow
logging in from the CLI via the --user and --password arguments as do
other actions requiring to be logged in.

Signed-off-by: Pierre-Yves Chibon <pingou@pingoured.fr>
@pypingou pypingou force-pushed the pypingou:user_password_argument branch from 054f61b to d9e738c Oct 21, 2019
@abompard abompard dismissed their stale review Oct 21, 2019

File added

@mergify mergify bot merged commit 1891d26 into fedora-infra:develop Oct 21, 2019
35 of 37 checks passed
35 of 37 checks passed
continuous-integration/jenkins/pr-head This commit is being built
Details
rawhide-integration running
Details
DCO DCO
Details
Rule: backport 5.0 (backport) Backports have been created
Details
Summary 3 rules match and 5 potential rules
Details
f29-build
Details
f29-diff-cover
Details
f29-docs
Details
f29-integration
Details
f29-integration-build
Details
f29-unit
Details
f30-build
Details
f30-diff-cover
Details
f30-docs
Details
f30-integration
Details
f30-integration-build
Details
f30-unit
Details
f31-build
Details
f31-diff-cover
Details
f31-docs
Details
f31-integration
Details
f31-integration-build
Details
f31-unit
Details
pip-build
Details
pip-diff-cover
Details
pip-docs
Details
pip-flake8
Details
pip-integration
Details
pip-integration-build
Details
pip-mypy
Details
pip-pydocstyle
Details
pip-unit
Details
rawhide-build
Details
rawhide-diff-cover
Details
rawhide-docs
Details
rawhide-integration-build
Details
rawhide-unit
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.