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
Bug 1902299: catalog mirror improvements #673
Bug 1902299: catalog mirror improvements #673
Conversation
@ecordell: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@ecordell: This pull request references Bugzilla bug 1902299, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest |
Depends on openshift/library-go#973 |
69b2381
to
264bbf3
Compare
this ensures that the mirrored manifests can still reference the manifestlist by digest
264bbf3
to
dbef35a
Compare
@ecordell: This pull request references Bugzilla bug 1902299, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
does not filter the mirrored images
dbef35a
to
5f2fd3e
Compare
/retest |
/lgtm |
/retest Quota issues |
/approve |
/retest |
2 similar comments
/retest |
/retest |
5f2fd3e
to
836f56f
Compare
836f56f
to
c615889
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits that can be addressed in a follow-up
/approve
|
||
// the old flag name is kept for backwards-compatibility. | ||
// if both old and new are specified, the value of the flag coming later will be used. | ||
flags.StringVar(&o.FilterOptions.FilterByOS, "filter-by-os", o.FilterOptions.FilterByOS, "Use --index-filter-by-os instead. A regular expression to control which index image is picked when multiple variants are available. Images will be passed as '<platform>/<architecture>[/<variant>]'. This does not apply to images referenced by the index.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest deprecating this flag, not to confuse users. Can be done in a follow-up.
@@ -144,8 +154,10 @@ func (o *MirrorCatalogOptions) Complete(cmd *cobra.Command, args []string) error | |||
src := args[0] | |||
dest := args[1] | |||
|
|||
if err := o.FilterOptions.Complete(cmd.Flags()); err != nil { | |||
return err | |||
// default to linux/arm64 for index image, which we generally expect to exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: comment says default to linux/arm64
but you default to linux/amd64
// default to linux/arm64 for index image, which we generally expect to exist | ||
pattern := o.FilterOptions.FilterByOS | ||
if len(pattern) == 0 { | ||
o.FilterOptions.FilterByOS = regexp.QuoteMeta(fmt.Sprintf("%s/%s", "linux", "amd64")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excessive, just pass linux/amd64
.
/retest |
1 similar comment
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ecordell, kevinrizza, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
6 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@ecordell: All pull requests linked via external trackers have merged: Bugzilla bug 1902299 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-4.6 |
@ecordell: #673 failed to apply on top of branch "release-4.6":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This fixes some issues with oc adm catalog mirror, post-appregistry:
registry.access.redhat.com/ubi8-minimal
--filter-by-os
was previously allowed to filter arches of mirrored content. This would break references to those images in the catalog, which point the manifestlist and not the manifest.--filter-by-os
now only filters the index image that is pulled and unpacked. To clarify this, a new flag--index-filter-by-os
was added and should be used instead.String()
is calculated on parsed references, some valid targets were failing validation. Minimal validation for a mirroring target now only checks that a hostname has been parsed successfully.These need to be backported to 4.6 as the address issues with mirroring index images (vs. previous workflows that assumed mirroring started with
oc adm catalog build
)