Only push updates that have all builds marked as signed #1011
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bowlofeggs
reviewed
Oct 7, 2016
| % if build.signed | ||
| <span class="glyphicon glyphicon-unlock"></span> | ||
| % else | ||
| <span class="glyphicon glyphicon-lock"></span> |
There was a problem hiding this comment.
I think the lock and unlock icons might be backwards here.
bowlofeggs
reviewed
Oct 7, 2016
| @@ -71,19 +72,27 @@ def push(username, password, cert_prefix, **kwargs): | |||
| updates.append(update) | |||
| click.echo(update) | |||
| else: | |||
| config_uri = '/etc/bodhi/production.ini' | |||
There was a problem hiding this comment.
It might be nice to make this non-hardcoded, especially if we want to be able to unit test this module.
There was a problem hiding this comment.
Fixed, made it an optional argument, which defaults to production.ini
puiterwijk
changed the title
puiterwijk
force-pushed
the
sign-request
branch
from
13ae946 to
b66dafb
Compare
|
This pull request is no longer WIP, and should be ready. |
|
My last commit makes us lock the updates as we're sending them off to the masher. |
puiterwijk
added
Critical
puiterwijk
force-pushed
the
sign-request
branch
2 times, most recently
from
32f45b1 to
ef48383
Compare
bowlofeggs
reviewed
Oct 11, 2016
| click.echo('\nLocking updates...') | ||
| for update in updates: | ||
| update.locked = True | ||
| update.date_locked = datetime.utcnow() |
There was a problem hiding this comment.
We might want to commit the transaction after this for loop is done. Otherwise, we will send the message to start the mash and commit the transaction, rather than committing the transaction and then starting the mash. I know this is a race we are likely to win, but it's a race that easy to avoid ☺
bowlofeggs
reviewed
Oct 11, 2016
| <td>Request</td><td>${self.util.request2html(update.request) | n}</td> | ||
| <td>Request</td><td>${self.util.request2html(update.request) | n} | ||
| % if not update.signed | ||
| <span class="glyphicon glyphicon-unlock"></span> |
There was a problem hiding this comment.
It might be good to add hoverover texts explaining what these icons mean, otherwise they might just be mysterious to people.
bowlofeggs
reviewed
Oct 11, 2016
| @@ -495,6 +495,7 @@ class Build(Base): | |||
| package_id = Column(Integer, ForeignKey('packages.id')) | |||
| release_id = Column(Integer, ForeignKey('releases.id')) | |||
| update_id = Column(Integer, ForeignKey('updates.id')) | |||
| signed = Column(Boolean, default=False) | |||
There was a problem hiding this comment.
I think it makes sense to go ahead and slap a nullable=False on this guy, just to be opinionated about it. (I think a lot of fields should have this, but I can deal with that another day).
bowlofeggs
reviewed
Oct 11, 2016
| from bodhi.server.models import Base, Build, Release | ||
|
|
||
| import logging | ||
| log = logging.getLogger('bodhi') |
There was a problem hiding this comment.
For PEP-8, let's regroup these imports:
import logging
import pprint
import fedmsg.consumers
from pyramid.paster import get_appsettings
from sqlalchemy import engine_from_config
from bodhi.server.util import transactional_session_maker
from bodhi.server.models import Base, Build, Release
That way I don't have to fix that when I get around to adding this new module to flake8's automatic test ☺
bowlofeggs
reviewed
Oct 11, 2016
| """ | ||
| config_key = 'signed_handler' | ||
|
|
||
| def __init__(self, hub, db_factory=None, *args, **kwargs): |
There was a problem hiding this comment.
Can you write a docblock here, describing the arguments? It would be helpful to know when/why the db_factory might be None.
There was a problem hiding this comment.
There will never be a db_factory. I just killed that.
bowlofeggs
reviewed
Oct 11, 2016
| self.settings = get_appsettings(config_uri) | ||
| engine = engine_from_config(self.settings, 'sqlalchemy.') | ||
| Base.metadata.create_all(engine) | ||
| self.db_factory = transactional_session_maker(engine) |
There was a problem hiding this comment.
Code like this is repeated around Bodhi, and actually appears twice in this pull request. I think it makes sense to make this a utility function and call it from the places than need the db_factory.
bowlofeggs
reviewed
Oct 11, 2016
|
|
||
| log.info("Signed Handler handling %s, %s" % (alias, topic)) | ||
|
|
||
| build_nvr = '%(name)s-%(version)s-%(release)s' % msg |
There was a problem hiding this comment.
Is the epoch not supposed to be considered here?
There was a problem hiding this comment.
I think this is taken as part of version.
There was a problem hiding this comment.
Even better: epoch is totally ignored by bodhi, everywhere. (and in koji build identification too btw)
There was a problem hiding this comment.
Hahah, well that sounds like something I should probably fix eventually. Since that's the case I agree we can ignore for this PR.
bowlofeggs
reviewed
Oct 11, 2016
| build = Build.get(build_nvr, session) | ||
| if not build: | ||
| log.info("Build was not submitted, skipping") | ||
| return |
There was a problem hiding this comment.
If a build gets signed before being submitted to Bodhi, how will the Build.signed column get updated? It seems like this would be the more common case, since I'd expect signing to happen shortly after the build. Or does the signing not happen until the build is submitted as an update by Bodhi typically?
There was a problem hiding this comment.
Signing is only done after the update is submitted to bodhi: the tagging into the pending-signing tag will trigger it.
|
Here's a migration starter-pack, but I think you should adjust it to make the signed column non-nullable. To do that, you probably need to do something like making the column nullable, going and setting all Builds to the default value (False, right?), then converting the column to be non-nullable: |
|
There seems to be some problem with the I'm going to try to reproduce this outside of your PR, and if I can I'll fix it in another PR. |
puiterwijk
force-pushed
the
sign-request
branch
4 times, most recently
from
81f9c94 to
21cfdae
Compare
|
I've been able to reproduce the traceback on the I'm going to go fix that and then come back to reviewing this PR so I can test it. |
|
It turns out that you can't query with strings on the release, and that you have to use the enum when querying. You need to replace We could get this merged more quickly if we didn't rely on me to do all the testing for this PR. The Vagrant environment is designed to be easy for contributors to get a development environment running without much hassle. All you need to do is install some packages, copy some files, and type "vagrant up" and off you go! See the README for details. |
|
I would be more inclined to test if the official methods of getting it running actually worked, as there's also a virtualenv method in the documentation taht doesn't work at all, and I reported that before. |
puiterwijk
force-pushed
the
sign-request
branch
from
21cfdae to
8754c4a
Compare
|
There seems to be an issue with how the transaction commit is happening: I think maybe we want |
bowlofeggs
reviewed
Oct 18, 2016
|
|
||
| def upgrade(): | ||
| op.add_column('builds', sa.Column('signed', sa.Boolean(), nullable=True, default=False)) | ||
| op.execute('''UPDATE "builds" SET signed=0;''') |
There was a problem hiding this comment.
We had an IRC chat about this, and @puiterwijk and I both think that setting existing builds to True is better than setting them to False because existing builds that are signed will be labeled as unsigned and will be unpushable. Falsely listing Builds as signed will be OK because the autosigner will still get to them and they will become signed eventually (i.e., we will rely on eventual consistency).
puiterwijk
force-pushed
the
sign-request
branch
from
8754c4a to
c19a04c
Compare
bowlofeggs
reviewed
Oct 18, 2016
| This signature is a randomly generated UUID used to de-duplicate | ||
| alerts and version information. This signature is random, it is | ||
| not based on any personally identifiable information. To create | ||
| a new signature, you can simply delete this file at any time. |
There was a problem hiding this comment.
Strange, I have a .vagrant folder, not a .vagrant.d and only .vagrant is in the .gitignore. Can you remove these files from your commit? I'll make a PR to add .vagrant.d to the .gitignore for you.
There was a problem hiding this comment.
P.S. I wonder if this is the way never Vagrant versions do it (since you are working on Rawhide)?
puiterwijk
force-pushed
the
sign-request
branch
from
c19a04c to
888cd55
Compare
bowlofeggs
reviewed
Oct 18, 2016
| agent=username, | ||
| ), | ||
| force=True, | ||
| ) |
There was a problem hiding this comment.
Could we move this fedmsg to be outside of the transaction so that the transaction is committed before the message is sent? It could be bad if the transaction.commit() fails and we sent the message already, or if the message is received and processed before the transaction is committed somehow.
There was a problem hiding this comment.
Hahah I think I was writing this comment right when you made the change, so my comment went on the code that was already changed. This looks good.
puiterwijk
force-pushed
the
sign-request
branch
2 times, most recently
from
2eb2df2 to
e42cb9b
Compare
|
The migration seems to fail: I suggest using sqlalchemy core to form the update query so that the right thing happens no matter what the backend is. |
puiterwijk
force-pushed
the
sign-request
branch
from
e42cb9b to
7b8b2a1
Compare
|
There are five failing tests in this PR, and I think they are all similar. There is too much output to list here, so I'll just list one: |
puiterwijk
force-pushed
the
sign-request
branch
from
7b8b2a1 to
c9592e2
Compare
Signed-off-by: Patrick Uiterwijk <puiterwijk@redhat.com>
puiterwijk
force-pushed
the
sign-request
branch
from
c9592e2 to
a2f449d
Compare
|
It sounds like it should be showing me which updates it's about to push, but it doesn't print any: |
bowlofeggs
reviewed
Oct 18, 2016
|
|
||
|
|
||
| if update_titles: | ||
| click.echo('\nSending masher.start fedmsg') |
There was a problem hiding this comment.
This block is over-indented. It should be four spaces instead of 8.
puiterwijk
force-pushed
the
sign-request
branch
from
a2f449d to
598ebc6
Compare
|
@puiterwijk There are still some failing unit tests. They are again long output, so I'll just show the last one (they all seem similar): |
This makes bodhi-push: - Use its own database connection - Only submit updates that are marked as signed Signed-off-by: Patrick Uiterwijk <puiterwijk@redhat.com>
…in koji Signed-off-by: Patrick Uiterwijk <puiterwijk@redhat.com>
puiterwijk
force-pushed
the
sign-request
branch
from
598ebc6 to
2c45641
Compare
bowlofeggs
approved these changes
Oct 18, 2016
There was a problem hiding this comment.
@puiterwijk This looks good to me, thanks so much for your patience with all the back and forth on this one. I think we've got something pretty nice here and I think releng will be happy with this huge improvement.
Nice work!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will make Bodhi play along nicely with RoboSignatory.