move diff calculations into cosa import
#4340
Conversation
This moves the RPM/Advisory diffing into `cosa import`, which means we'll get that info even when importing from another source. It also changes the code such that we'll only attempt to add that info if there was something to diff against. i.e. for a first build in a stream (like when doing a local dev build for the first time) now we won't error out. Fixes coreos#4336
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the RPM/advisory diffing logic by moving it from cmd-build-with-buildah into cmd-import. This is a good change as it centralizes the logic and allows it to be used when importing from other sources. The change also correctly handles the case of a first build, preventing errors when there's no previous build to diff against. The implementation looks solid. I have one minor suggestion to improve code style.
| # advisory diffs into the meta.json and log the RPM diff, but | ||
| # only if we have something to diff against. | ||
| if args.parent_build or len(builds.get_builds()) > 1: | ||
| starting_buildid = args.parent_build if args.parent_build else builds.get_previous() |
There was a problem hiding this comment.
This line can be simplified by using the or operator, which is more concise and idiomatic in Python for this type of assignment.
| starting_buildid = args.parent_build if args.parent_build else builds.get_previous() | |
| starting_buildid = args.parent_build or builds.get_previous() |
There was a problem hiding this comment.
ehh. I'm lukewarm on this suggestion. Will implement if reviewers like it.
There was a problem hiding this comment.
I've started using that idiom more but yeah, it can look weird if you're not used to it. Fine as is.
| # advisory diffs into the meta.json and log the RPM diff, but | ||
| # only if we have something to diff against. | ||
| if args.parent_build or len(builds.get_builds()) > 1: | ||
| starting_buildid = args.parent_build if args.parent_build else builds.get_previous() |
There was a problem hiding this comment.
I've started using that idiom more but yeah, it can look weird if you're not used to it. Fine as is.
| 'pkgdiff': diff_data.get('pkgdiff'), | ||
| 'advisories-diff': diff_data.get('advisories') | ||
| }) | ||
| meta.write() |
There was a problem hiding this comment.
Hmm right, I was thinking that we could directly write the updated meta.json on first shot, but indeed cosa diff doesn't know how to work with staged builds. And meh... doesn't seem worth it for this.
This moves the RPM/Advisory diffing into
cosa import, which means we'll get that info even when importing from another source.It also changes the code such that we'll only attempt to add that info if there was something to diff against. i.e. for a first build in a stream (like when doing a local dev build for the first time) now we won't error out.
Fixes #4336