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

addbuilds and removebuilds #3125

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Conversation

tdawson
Copy link
Contributor

@tdawson tdawson commented Apr 3, 2019

New edit options for bodhi client

  • bodhi updates edit --addbuilds
  • bodhi updates edit --removebuilds
    Given a comma seperated list of build nvr's, it adds, or removes, those builds from the update.

@tdawson tdawson requested a review from a team as a code owner April 3, 2019 13:44
@tdawson tdawson force-pushed the addbuilds branch 2 times, most recently from fed8e96 to 1454f88 Compare April 3, 2019 21:40
@cverna
Copy link
Contributor

cverna commented Apr 4, 2019

Change LGTM, but you might want to squash your commit to a single commit.

@@ -98,6 +98,8 @@ def _set_logging_debug(ctx, param, value):
new_edit_options = [
click.option('--user'),
click.option('--password', hide_input=True),
click.option('--addbuilds', help='Add Comma-separated list of build nvr'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that this are the shared options between the update new and the update edit command. I think these options are only really useful for edit. So they should probably be added directly to the edit method. What do you think ?

see https://github.com/fedora-infra/bodhi/blob/develop/bodhi/client/__init__.py#L440 for an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree.

@cverna
Copy link
Contributor

cverna commented Apr 5, 2019

I am not 100% sure why but it seems that this change broke some existing tests. You can run pytest-3 -vvv bodhi/tests/client/test___init__.py in the vagrant environment to see the failures.

@tdawson
Copy link
Contributor Author

tdawson commented Apr 5, 2019

I needed to remove addbuilds and removebuilds from kwargs each time.
I was removing them whenever they were called, which allowed my tests to pass. But I didn't realize that was the reason all the other tests were now failing.
Fixed.

@cverna
Copy link
Contributor

cverna commented Apr 5, 2019

I am sorry I just thought about one last things 🤦‍♂️ .We will want to update the man page of the bodhi cli with the new options. https://github.com/fedora-infra/bodhi/blob/develop/docs/user/man_pages/bodhi.rst

Other than that I ran the tests locally and everything looked good 👍

Signed-off-by: Troy Dawson <[email protected]>
@tdawson
Copy link
Contributor Author

tdawson commented Apr 5, 2019

Good catch.
Manpage for bodhi updated.

@cverna
Copy link
Contributor

cverna commented Apr 8, 2019

👍

@cverna cverna merged commit ff738cf into fedora-infra:develop Apr 8, 2019
@bowlofeggs
Copy link
Contributor

In the future, please don't merge pull requests unless the CI passes (unless the pull request is critical, like a security issue or something along those lines). When CI isn't passing, we should focus on fixing that and hold all pull requests from merging until that issue is resolved.

@bowlofeggs
Copy link
Contributor

This patch is planned for inclusion in the upcoming 4.0.0 release: #3221

@bowlofeggs
Copy link
Contributor

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.

3 participants