WIP: WRKLDS-1599: Update to distribution/[email protected] #532
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@tchap: This pull request references WRKLDS-1599 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tchap
force-pushed
the
distribution-v3
branch
2 times, most recently
from
56f50e6 to
97e311b
Compare
|
PR needs rebase. 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-sigs/prow repository. |
openshift-merge-robot
added
the
needs-rebase
|
@tchap: The following tests 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-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| "github.com/openshift/library-go/pkg/image/reference" | ||
| "github.com/openshift/library-go/pkg/image/registryclient" | ||
| registryclient "github.com/openshift/library-go/pkg/image/registryclient/v2" |
There was a problem hiding this comment.
isn't this package already called registryclient?
| // imageIndexJSON actually contains the same manifests as manifestListJSON, | ||
| // so the same manifests can be used during testing. | ||
| // | ||
| //go:embed testdata/image-index.json |
There was a problem hiding this comment.
why did we need to create a copy of manifestListJSON ? 🤔
| _, isIndex := manifest.(*ocischema.DeserializedImageIndex) | ||
| _, isList := manifest.(*manifestlist.DeserializedManifestList) | ||
| if isIndex || isList { | ||
| if importMode != "" && importMode != imageapi.ImportModeLegacy { |
There was a problem hiding this comment.
I think placing the these checks into a variable makes the condition considerably easier to read.
i.e.:
legacyManifestListImport := importMode == "" || importMode == imageapi.ImportModeLegacy
// ...
if !legacyManifestListImport {
// ...
}
What do you think?
| return manifestToImage(manifest, d) | ||
| } | ||
|
|
||
| manifest, d, err = selectManifestForPlatform(ctx, manifest, ref, s, preferArch, preferOS) |
There was a problem hiding this comment.
This is a much better name for manifestFromManifestList, thanks!
|
I really like the refactorings you made here @tchap. |
|
@flavianmissi I see you woke up after some time 🙂 I think that I put this to rest for now because I was blocked on rebase for apiserver-library-go or something. This change is rather complex across multiple repositories. But I can try to check the status this week or potentially the next, I am working on some Kubernetes stuff right now. |
|
no rush, I'll be off most of november so won't be able to review anything more anyway. let's catch up later. |
4 participants
Move to use upstream distribution/v3 instead of our private fork.
This is blocked by openshift/library-go#1972