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
OCPNODE-1580: Add --print-mirror-instructions to oc adm release mirror to allow idms instructions #1341
OCPNODE-1580: Add --print-mirror-instructions to oc adm release mirror to allow idms instructions #1341
Conversation
4b440af
to
84cf843
Compare
/hold |
/retest-required |
@ardaguclu PTAL. update the printout instruction using versions, the example output for 4.5.4 and 4.14:
|
/hold cancel openshift/installer#6235 allows |
/retest-required |
719abfe
to
edaf982
Compare
/retest-required |
@ardaguclu PTAL |
/test e2e-metal-ipi-ovn-ipv6 |
This PR looks good to me but I think we need pre-merge testing to move forward. /lgtm |
Although |
/retest-required |
/test e2e-metal-ipi-ovn-ipv6 |
edaf982
to
093d9b1
Compare
/test e2e-metal-ipi-ovn-ipv6 |
Sounds like a good plan. But I don't think we would need this hacky version check in here https://github.com/openshift/oc/pull/1341/files#diff-5cff67bbeb34fe73a26ed00f0b2099417027140a1907d25a7d3ac3f0e2c9d982R905. Because user can specify whichever output format they want. What about we propose a new flag as you said, namely oc/pkg/cli/admin/release/mirror.go Line 254 in 0e39068
In the future, we can change default to idms? |
Sounds good to me. I would rework this. |
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'll finish detailed review but left a minor comment now.
pkg/cli/admin/release/mirror.go
Outdated
@@ -162,6 +169,7 @@ func NewMirror(f kcmdutil.Factory, streams genericclioptions.IOStreams) *cobra.C | |||
flags.BoolVar(&o.ApplyReleaseImageSignature, "apply-release-image-signature", o.ApplyReleaseImageSignature, "Apply release image signature to connected cluster.") | |||
flags.StringVar(&o.ReleaseImageSignatureToDir, "release-image-signature-to-dir", o.ReleaseImageSignatureToDir, "A directory to export release image signature to.") | |||
|
|||
flags.StringVar(&o.PrintImageSourceInstructions, "print-mirror-instructions", o.PrintImageSourceInstructions, "Print instructions of ImageContentSourcePolicy or ImageDigestMirrorSet for using images from mirror registries. Allowed value is 'icsp' or 'idms'. Default value is icsp.") |
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 know, it wasn't supported before but can we also add none
to not print anything?
@QiWang19 What is the plan changing the default from Maybe we should a warning about "use idms instead icsp" when icsp is printed. But that fully depends on the calendar of migration from icsp to idms. |
|
||
var sources []operatorv1alpha1.RepositoryDigestMirrors | ||
var ( |
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 think, there is no shortcut return if printImageSourceInstructions
is none.
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.
fixed.
Add --print-mirror-instructions flag to command oc adm release. The print out instructions of the command will keep it as is if the flag is unset or set to `icsp`. If set to 'idms', update instruction output of `oc adm release mirror` to ImageDigestMirrorSet example. The new ImageDigestMirrorSet examples are: - use `imageDigestSources` field in install-config.yaml[1] that will creates ImageDigestMirrorSet manifests, - manifest example of ImageDigestMirrorSet CRD [1] Add `imageDigestSources` to install-config: openshift/installer#6235 result of this patch: ``` $ oc adm release mirror --print-mirror-instructions=idms -a /home/qiwan/cluster/mysecret --from=quay.io/openshift-release-dev/ocp-release:4.13.2-x86_64 --to=localhost:5000/ocp4/openshift4 --to-release-image=localhost:5000/ocp4/openshift4:4.13-x86_64 --dry-run To use the new mirrored repository to install, add the following section to the install-config.yaml: imageDigestSources: - mirrors: - localhost:5000/ocp4/openshift4 source: quay.io/openshift-release-dev/ocp-release - mirrors: - localhost:5000/ocp4/openshift4 source: quay.io/openshift-release-dev/ocp-v4.0-art-dev To use the new mirrored repository for upgrades, use the following to create an ImageDigestMirrorSet: apiVersion: config.openshift.io/v1 kind: ImageDigestMirrorSet metadata: name: example spec: imageDigestMirrors: - mirrors: - localhost:5000/ocp4/openshift4 source: quay.io/openshift-release-dev/ocp-release - mirrors: - localhost:5000/ocp4/openshift4 source: quay.io/openshift-release-dev/ocp-v4.0-art-dev ``` Signed-off-by: Qi Wang <qiwan@redhat.com>
@ardaguclu PTAL |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, QiWang19 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 |
/retitle OCPNODE-1580: Add --print-mirror-instructions to oc adm release mirror to allow idms instructions |
@QiWang19: This pull request references OCPNODE-1580 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 kubernetes/test-infra repository. |
/test e2e-aws-ovn-upgrade |
Do we need to tag |
This is a feature and only TRT can add this label. I think, you should add this PR into TRT's queue. |
/label jira/valid-bug |
@QiWang19: all tests passed! 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. |
story: https://issues.redhat.com/browse/OCPNODE-1580
Add
--print-mirror-instructions
flag to command oc adm release.The printout instructions of the command will keep it as is if the flag is unset or set to
icsp
.If set to
idms
, update the instruction output ofoc adm release mirror
to ImageDigestMirrorSet example.The new ImageDigestMirrorSet examples are:
imageDigestSources
field in install-config.yaml[1] that will creates ImageDigestMirrorSet manifests,[1] Add
imageDigestSources
to install-config: openshift/installer#6235result of this patch:
If specify
--print-mirror-instructions=icsp
print out deprecated message to direct users to use idms.