This repository has been archived by the owner on Oct 13, 2023. It is now read-only.
[ART-2617] use upstream-equivalent FROM images when a flag is set in group.yaml #646
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
sosiouxme
changed the title
sosiouxme
suggested changes
Sep 23, 2022
There was a problem hiding this comment.
couldn't get it to work as expected, will probably need a better review later.
when you are refactoring into smaller methods, that's a time i'd really like to see some unit tests added for them (which this file is sorely lacking, due in part to its giant methods).
doozerlib/distgit.py
Outdated
| return stream.image | ||
|
|
||
| # When canonical_builders_from_upstream flag is set, try to match upstream FROM | ||
| original_parent = original_parent |
doozerlib/distgit.py
Outdated
| # specific sha. | ||
| # If you are here trying to figure out how to change this behavior, you should | ||
| # consider using 'from!:' in the assembly metadata for this component. This will | ||
| # all you to fully pin the parent images (e.g. {'from!:' ['image': <pullspec>] }) |
There was a problem hiding this comment.
Suggested change
| # all you to fully pin the parent images (e.g. {'from!:' ['image': <pullspec>] }) | |
| # allow you to fully pin the parent images (e.g. {'from!:' ['image': <pullspec>] }) |
doozerlib/distgit.py
Outdated
| labels = json.loads(out)['config']['config']['Labels'] | ||
| builder_tag = f'{labels["version"]}-{labels["release"]}' | ||
|
|
||
| # Verify wether the image exists |
There was a problem hiding this comment.
Suggested change
| # Verify wether the image exists | |
| # Verify whether the image exists |
doozerlib/distgit.py
Outdated
Comment on lines
1630
to
1650
| upstream_equivalent = f'registry-proxy.engineering.redhat.com/rh-osbs/' \ | ||
| f'openshift-golang-builder:{builder_tag}' | ||
| cmd = f'oc image info {upstream_equivalent} --filter-by-os linux/amd64 -o json' |
There was a problem hiding this comment.
Actually I don't want to assume this is just for golang-builder (even though that's the only use case for now), and I don't want to look things up by registry repo. With one more label (com.redhat.component) we have the NVR and can look up anything in brew!
That does leave the question of how to make sure we're not substituting for members, though, and I think the easiest rule to follow here is... only if the image was specified with
builder:
- stream: ...
then it's eligible for override by whatever the upstream is pointing at, if that has the labels of a brew NVR. (And I think that's already the only path to this code, right?) I thought it might be useful to be able to do this for base images too but after a bit of consideration, not really.
There was a problem hiding this comment.
Yes, this code is executed only for images specified with builder: stream (and for assembly stream builds...)
There was a problem hiding this comment.
Fixed in ac3e683
| @@ -1612,6 +1612,50 @@ def _mapped_image_for_assembly_build(self, parent_images, i): | |||
| unique_pullspec += f':{parent_build_nvr["version"]}-{parent_build_nvr["release"]}' | |||
| return unique_pullspec | |||
|
|
|||
| def _mapped_image_from_stream(self, image, original_parent, dfp): | |||
There was a problem hiding this comment.
could there be some logger.debug statements in here, and probably even a logger.info for when the upstream does override our config? in my tests it wasn't easy to see why the upstream override isn't happening as expected. actually i don't think it's even getting here
There was a problem hiding this comment.
Added in 98d9760
doozerlib/distgit.py
Outdated
|
|
||
| except ChildProcessError: | ||
| # It doesn't. Emit a warning and do typical stream resolution | ||
| logger.warning(f'Could not match upstream parent {original_parent}') |
doozerlib/distgit.py
Outdated
| except ChildProcessError: | ||
| # It doesn't. Emit a warning and do typical stream resolution | ||
| logger.warning(f'Could not match upstream parent {original_parent}') | ||
| stream = self.runtime.resolve_stream(image.stream) |
Build #10 |
locriandev
force-pushed
the
ART-2617
branch
from
6e8fd6f
to
34cb193
Compare
Build #11 |
Build #12 |
Build #13 |
sosiouxme
approved these changes
Sep 29, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Ref. https://issues.redhat.com/browse/ART-2617