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

API: Create/edit updates from Koji tags #3228

Open
wants to merge 8 commits into
base: develop
from

Conversation

4 participants
@nphilipp
Copy link
Member

commented May 14, 2019

This PR contains code to let users submit new updates or edit existing ones for a Koji (side) tag, i.e. it would pull the latest builds in the tag into the update and remember the tag with the update. For editing (not fully implemented yet), it would work like this: if the list of submitted builds is non-empty, do things just as before, if it is empty, consult Koji again for a list of builds.

NB: it uses the first two commits from PR#3168, so it would be good if this one would be merged first.

@nphilipp nphilipp force-pushed the nphilipp:develop--update-from-sidetag-api branch 4 times, most recently from c3e307f to c919086 May 17, 2019

@nphilipp nphilipp removed the WIP label May 21, 2019

@nphilipp nphilipp marked this pull request as ready for review May 21, 2019

@nphilipp nphilipp requested a review from fedora-infra/bodhi as a code owner May 21, 2019

@nphilipp

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

NB: This is based off of #3232 and shares the first two commits with #3168. I'd consider #3232 to be blocking it, but not the latter—it doesn't matter in which order they go in, I'll rebase the respective other accordingly and update its migration script.

@nphilipp nphilipp added this to To do in CI Gating via automation May 21, 2019

@nphilipp nphilipp moved this from To do to Need Review in CI Gating May 21, 2019

@nphilipp nphilipp force-pushed the nphilipp:develop--update-from-sidetag-api branch 2 times, most recently from 98d611b to 62314b1 May 23, 2019

@nphilipp

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

To bring some order into my PRs, this one is now based off of #3168, to avoid having to double-check common commits and migration script ordering between them.

@nphilipp nphilipp force-pushed the nphilipp:develop--update-from-sidetag-api branch from 62314b1 to 0974f96 May 24, 2019

@nphilipp nphilipp force-pushed the nphilipp:develop--update-from-sidetag-api branch 4 times, most recently from d3a965a to 51500e3 May 28, 2019

@cverna

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@nphilipp You will probably want to squash your commits 😄

Show resolved Hide resolved ...erver/migrations/versions/d3f8bd499ecd_add_from_tag_column_to_updates.py
Show resolved Hide resolved bodhi/server/models.py Outdated
Show resolved Hide resolved bodhi/server/models.py Outdated
Show resolved Hide resolved bodhi/server/validators.py Outdated
Show resolved Hide resolved bodhi/server/validators.py
Show resolved Hide resolved bodhi/server/validators.py Outdated
Show resolved Hide resolved bodhi/server/validators.py Outdated
@nphilipp

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

@nphilipp You will probably want to squash your commits

Of course -- do you think the first one "Use less ambiguous validator names." should be separate or part of the squashed feature?

@nphilipp nphilipp force-pushed the nphilipp:develop--update-from-sidetag-api branch from 51500e3 to 311e065 Jun 6, 2019

@nphilipp

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

@cverna This latest push should address your requests. Mind that the order GitHub lists the commits is wrong (why ever), the first three are:

bef23ece Correct location of request type.
53cffcf0 Don't mention obsolete string types.
6af83d8f Use less ambiguous validator names.

Again, I'm not sure if the last of these shouldn't be squashed with the feature commits. What do you think?

# We don't want the new update to obsolete the existing one.
self.db.delete(Update.query.one())

update = self.get_update(builds=None, from_tag='f17-updates-candidate')

This comment has been minimized.

Copy link
@abompard

abompard Jun 6, 2019

Member

This fails in CI because the code does not expect builds to be None, only a list (that can be empty). I suggest to either use [] here or to handle in the validation code the case where builds is None.

This comment has been minimized.

Copy link
@nphilipp

nphilipp Jun 7, 2019

Author Member

Where does it fail and how? AIUI, it uses BaseTestCaseMixin.get_update() which has this:

        if builds:
            if isinstance(builds, list):
                builds = ','.join(builds)
            if not isinstance(builds, str):
                builds = builds.encode('utf-8')
            update['builds'] = builds

I.e. if builds is set to None, it should simply be missing from the returned dictionary.

This comment has been minimized.

Copy link
@abompard

abompard Jun 7, 2019

Member

In the CI logs you'll see:

13:55:12  Traceback (most recent call last):
13:55:12    File "/usr/lib/python3.7/site-packages/pyramid/tweens.py", line 41, in excview_tween
13:55:12      response = handler(request)
13:55:12    File "/usr/lib/python3.7/site-packages/pyramid/router.py", line 148, in handle_request
13:55:12      registry, request, context, context_iface, view_name
13:55:12    File "/usr/lib/python3.7/site-packages/pyramid/view.py", line 667, in _call_view
13:55:12      response = view_callable(context, request)
13:55:12    File "/usr/lib/python3.7/site-packages/pyramid/config/views.py", line 169, in __call__
13:55:12      return view(context, request)
13:55:12    File "/usr/lib/python3.7/site-packages/pyramid/config/views.py", line 188, in attr_view
13:55:12      return view(context, request)
13:55:12    File "/usr/lib/python3.7/site-packages/pyramid/config/views.py", line 214, in predicate_wrapper
13:55:12      return view(context, request)
13:55:12    File "/usr/lib/python3.7/site-packages/pyramid/viewderivers.py", line 384, in authdebug_view
13:55:12      return view(context, request)
13:55:12    File "/usr/lib/python3.7/site-packages/pyramid/viewderivers.py", line 325, in secured_view
13:55:12      return view(context, request)
13:55:12    File "/usr/lib/python3.7/site-packages/pyramid/viewderivers.py", line 436, in rendered_view
13:55:12      result = view(context, request)
13:55:12    File "/usr/lib/python3.7/site-packages/pyramid/viewderivers.py", line 144, in _requestonly_view
13:55:12      response = view(request)
13:55:12    File "/usr/lib/python3.7/site-packages/cornice/service.py", line 493, in wrapper
13:55:12      validator(request, **args)
13:55:12    File "/bodhi/bodhi/server/validators.py", line 90, in validator
13:55:12      f(request, **kwargs)
13:55:12    File "/bodhi/bodhi/server/validators.py", line 203, in validate_build_nvrs
13:55:12      for build in request.validated.get('builds', []):
13:55:12  TypeError: 'NoneType' object is not iterable

The builds key is set to None, so get() does not fallback on [], therefore the crash.

@abompard

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

An improper testcase crashes CI (I've suggested possible fixes in a line comment), other than that the code looks good, thanks :-)

@cverna

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

@cverna This latest push should address your requests. Mind that the order GitHub lists the commits is wrong (why ever), the first three are:

bef23ece Correct location of request type.
53cffcf0 Don't mention obsolete string types.
6af83d8f Use less ambiguous validator names.

Again, I'm not sure if the last of these shouldn't be squashed with the feature commits. What do you think?

I think renaming the validators can be squashed with the feature commit since it is kind of related.

@nphilipp

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

@cverna > I think renaming the validators can be squashed with the feature commit since it is kind of related.

Will do.

@nphilipp nphilipp force-pushed the nphilipp:develop--update-from-sidetag-api branch from 311e065 to d8ea887 Jun 7, 2019

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Can you move the "Don't mention obsolete string types." commit to another pull request, since it's unrelated to the other changes here? Alternatively, please squash the other commits to one and then this can stay here.

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

flake8 is upset about some lines in this PR:

11:08:37  ./bodhi/tests/server/test_validators.py:789:36: W504 line break after binary operator
11:08:37          assert (str(excinfo.value) ==
11:08:37                                     ^
11:08:37  ./bodhi/tests/server/test_validators.py:790:1: W191 indentation contains tabs
11:08:37  				"Encountered error while requesting tagged builds from Koji: 'foo'")
11:08:37  ^
11:08:37  ./bodhi/tests/server/test_validators.py:790:1: E101 indentation contains mixed spaces and tabs
11:08:37  				"Encountered error while requesting tagged builds from Koji: 'foo'")
11:08:37  ^
11:08:37  ./bodhi/tests/server/test_validators.py:790:5: E128 continuation line under-indented for visual indent
11:08:37  				"Encountered error while requesting tagged builds from Koji: 'foo'")
11:08:37      ^
11:08:37  ./bodhi/tests/server/test_validators.py:792:1: E101 indentation contains mixed spaces and tabs
11:08:37          # check type of wrapped exception
11:08:37  ^
Show resolved Hide resolved bodhi/server/models.py Outdated
Show resolved Hide resolved bodhi/server/validators.py
Show resolved Hide resolved bodhi/server/validators.py
Show resolved Hide resolved bodhi/server/validators.py
Show resolved Hide resolved bodhi/server/validators.py Outdated
@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

It would also be ideal to move the commit "Correct location of request type." to its own PR. It would make this one simpler to review.

@nphilipp

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

I'll move the standalone commits out into their own PR.

@nphilipp nphilipp force-pushed the nphilipp:develop--update-from-sidetag-api branch from d8ea887 to d96f234 Jun 17, 2019

@nphilipp

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

The first three commits are covered in their own PR #3320.

@nphilipp nphilipp force-pushed the nphilipp:develop--update-from-sidetag-api branch 3 times, most recently from b2482a5 to 54c5ffe Jun 17, 2019

nphilipp added some commits May 10, 2019

Add validate_from_tag().
This validator ensures that if from_tag is supplied in the request, it
is a valid Koji tag.

Signed-off-by: Nils Philippsen <nils@redhat.com>
Make DevBuildsys.listTagged() honor `latest` keyword.
Previously, it could return multiple builds of the same component in a
tag. This required updating tests which expected these wrong results.

Signed-off-by: Nils Philippsen <nils@redhat.com>
Add validate_builds_or_from_tag_exist().
This validator checks that at minimum one of `builds` or `from_tag` is
supplied as parameters and isn't empty.

Signed-off-by: Nils Philippsen <nils@redhat.com>
Accept from_tag alternatively to builds.
This is to let users create updates from all latest builds of a Koji
tag.

Signed-off-by: Nils Philippsen <nils@redhat.com>
Add Update.from_tag.
This column stores the tag from which the set of builds of an update was
filled initially.

Signed-off-by: Nils Philippsen <nils@redhat.com>
Add from_tag parameter to get_updates().
This allows testing updates where the builds should be filled from all
latest builds in a Koji tag.

Signed-off-by: Nils Philippsen <nils@redhat.com>
Create updates from a Koji tag.
This lets users specify a Koji tag instead of an explicit set of builds,
from which the latest component builds will be pulled.

Signed-off-by: Nils Philippsen <nils@redhat.com>
Move Koji tag expansion to validate_from_tag().
This finds the latest builds in a tag in the from_tag validator and adds
them to request.validated. This is necessary because the list of builds
is further used in validate_acls() to decide if a user is allowed to
submit an update for the involved builds.

Signed-off-by: Nils Philippsen <nils@redhat.com>

@nphilipp nphilipp force-pushed the nphilipp:develop--update-from-sidetag-api branch from 54c5ffe to 78ec4f7 Jun 19, 2019

@nphilipp

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

Apparently GH doesn't let me resolve this without dismissing. 😞

The accompanying comment documents why this construct is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.