-
Notifications
You must be signed in to change notification settings - Fork 55
fix: resolved bug for ocmirrorv2 from 4.19 #412
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
base: main
Are you sure you want to change the base?
Conversation
mohammedzee1000
left a comment
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.
Also if i remember correctly, the mirroring is also done for other disconnected setups @veera-damisetti can you please point to this
mohammedzee1000
left a comment
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.
Are we going to handle the disconnected install for @veera-damisetti setups in this PR as well?
|
@isumitsolanki |
|
@ksdeekshith, one thing is that I think the release image registry should be an optional field, as it's not required if we are testing with released OCP versions. This usually only becomes a problem with unreleased OCP versions. |
|
hi we need to resume this PR work soon before all current supported versions go out of scope |
mohammedzee1000
left a comment
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.
Looks good. The only minor crib that I can think of, really, is maybe we could use vars to reduce the repetition of some of the paths, such as {{ user_home.stdout }}/ocpinst_disconnected being repeated multiple times across the files.
But I would not consider it a blocker for merge.
For the line too long on comments, maybe we can consider restructuring the comment lines such as moving the line above or below the line the comment is for or maybe spliting the comment accross multipe lines.
| oc mirror --config {{ user_home.stdout }}/ocpinst/imageset.yaml docker://{{ disconnected.registry.url}} --ignore-history{{ ' --continue-on-error' if disconnected.mirroring.oc_mirror.oc_mirror_args.continue_on_error == True }} \ | ||
| {{ ' --source-skip-tls' if disconnected.mirroring.oc_mirror.oc_mirror_args.source_skip_tls == True }} | ||
| # ignore-history set by default for idempotency | ||
| oc mirror --v2 --config {{ user_home.stdout }}/ocpinst_disconnected/imageset.yaml --workspace file://{{ user_home.stdout }}/ocpinst_disconnected docker://{{ disconnected.registry.url }} --dry-run |
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.
Good
mohammedzee1000
left a comment
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.
lgtm
This PR will fix the oc mirror v2 from 4.19 for disconnected setups.
Note: This is verified for UPI installations. But not for ABI as of now. It will be verified based on the requirement.