freshmaker

#75 Restructure Docker image handling and support multiple advisories in a rebuild and multiple packages in single advisory.
Merged by jkaluza. Opened by jkaluza.
jkaluza/freshmaker multiple-events  into  master

Download 75.patch

Problems this PR solves

  • Current BrewSignRPMEvent and BrewSignRPMEventHandler is specific to single signed RPM, but in the end it rebuilds Docker image with all the RPMs from advisory. This makes it hard to track the advisory rebuild in database and API, because it is stored there as "BrewSignRPMEvent". The real event we are building based on is when "all RPMs in advisory are signed" and not when "single RPM is signed".
  • When advisory contains multiple packages, we are completely ignoring them and just rebuild based on the last BrewSignRPMEvent. So if there is httpd and apr in advisory and apr is signed as the last one, we are rebuilding docker image just with "apr-*" RPMs, and not with httpd.
  • When there are multiple concurrent advisories, for example httpd and nss advisory, we are not rebuilding httpd advisory with updated nss packages.

How are these problems solved

  • New ErrataAdvisoryRPMsSignedEvent is introduced. This event represents the state of Errata advisory when all its RPMs are signed.
  • Scope of BrewSignRPMHandler is changed heavily. It now only checks whether all the RPMs in advisory in which the signed RPM belongs to are signed. If they are signed, it emits internal ErrataAdvisoryRPMsSignedEvent which is handled by the ErrataAdvisoryRPMsSignedEventHandler. This handler does the rebuild work done by BrewSignRPMHandler previously.
  • ErrataAdvisoryRPMsSignedEventHandler works similarly like old BrewSignRPMHandler, but it finds out Docker images to rebuild for every package in advisory (not just the single one) and also generates the ODCS compose for every package in advisory.
  • ErrataAdvisoryRPMsSignedEventHandler also finds out the list of extra ErrataAdvisoryRPMsSignedEvent which were rebuilding the same Docker image as the currently handled event. For example, if we rebuilt image foo as a result of NSS advisory and now we are rebuilding httpd docker image which depends on foo, ErrataAdvisoryRPMsSignedEventHandler will trigger rebuild of httpd and foo images with the packages from NSS advisory and also with packages from httpd advisory.
  • This is done by introduction of models.Event.released bool which if False until the Errata is not in SHIPPED_LIVE state. Freshmaker listens for the errata.activity.state.change messages to update this bool.
  • Freshmaker tries to find out those extra_events in all unreleased events - the events for which the errata is not in SHIPPED_LIVE state.

rebased

rebased

What does "od" stand for here? Should this be "to"?

This should actually be "of" :)

I would reword this to:
"No Docker images were found to rebuild"

This is fine but it assumes that there have been no other changes to the db session other than potentially creating the event. I would just be cautious about this and if you think of a better way to avoid this, that would be preferred.

That's good point, will rework this part of code.

1 new commit added

  • Add tests for restructured Docker image handling.

+1

I guess I'd reword "No container images to rebuild for rpm advisory XYZ"

Hardcoding "-candidate" is worthy of at least a TODO to rework...

Hm, this is copy-paste from old code. I can rewrite that a way it will try "-candidate" as first guess and if it's not found, try other tags.

FWIW added a few comments but it's just hard to follow through this whole change. There seem to be a lot of pieces that could have been split off into smaller chunks/commits that would let me follow the thought process of creating this PR in the first place. I.e. add a new code, then add a few lines using it in a separate commit & remove the old code etc.

The way this is I am forced to jump up and down the PR as I follow the code. I might be able to review pieces, but if there is a major thought flaw I wouldn't be able to spot it IMO.

So I'll just say +1

Similar thought from me. @jkaluza explained background of this pr in irc, that is much helpful for me to understand. I think if put those words into code as comment or docstring, it would be good for review and others to understand easily.

od -> old ?

As we disucced, this handler is used to set Event.released=True when state is changed to SHIPPED_LIVE. This content seems not relative to that purpose.

Name ErrataAdvisoryStateChangedHandler is too general. For the purpose of what this handler does, would it better to change to another more specific name? Thus, by combining the name and method can_handle, it would be easier, at least for me I think, to understand that "this handler aims to set Event.released=True when ErrataAdvisoryStateChangedEvent happens". Regarding why should do this, a brief explanation could be put into docstring.

2 new commits added

  • Add tests for restructured Docker image handling.
  • Restructure Docker image handling and support multiple advisories in a rebuild and multiple packages in single advisory.

This would log content, e.g.

Ignoring Errata advisory RHEA-2014:0845 state change to QE, because it is not SHIPPED_LIVE

may be reworded to ?

Ignoring Errata advisory RHEA-2014:0845, because its state is not SHIPPED_LIVE.

Pull-Request has been merged by jkaluza
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.