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
no-jira: azure: Skip image upload if env var is set #8283
base: master
Are you sure you want to change the base?
Conversation
@rna-afk: This pull request explicitly references no 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. |
}) | ||
if err != nil { | ||
return err | ||
} |
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.
need to extend this so you also skip the gallery image and gallery image version
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.
Maybe you can move the image/gallery related calls to a function and either:
if _, ok := os.LookupEnv("OPENSHIFT_INSTALL_SKIP_IMAGE_UPLOAD"); !ok {
if err := createVMImage(ctx, createImageInput{...}); err != nil {
return err
}
}
Or always call it but check the env var inside createVMImage
and return immediately if it's set.
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 feel like doing this is more easier than passing a bunch of inputs to another function just for an extra if condition.
190ff46
to
199712b
Compare
Should a validation be added when you skip image uploading that a marketplace image has been specified? |
199712b
to
9bccf7b
Compare
/approve |
Just so we're all on the same page, this is only for development work. As with all env vars, this should not be used by customers. |
/lgtm |
/hold Revision 9bccf7b was retested 3 times: holding |
/hold cancel |
/hold Revision 9bccf7b was retested 3 times: holding |
9bccf7b
to
311170c
Compare
311170c
to
0d0cd69
Compare
/retest |
/hold cancel |
2214683
to
dbef392
Compare
/lgtm Tested and confirmed to work. |
/lgtm cancel Worker nodes are failing. |
@@ -628,12 +629,22 @@ func ValidateForProvisioning(client API, ic *types.InstallConfig) error { | |||
allErrs = append(allErrs, validateResourceGroup(client, field.NewPath("platform").Child("azure"), ic.Azure)...) | |||
allErrs = append(allErrs, ValidateDiskEncryptionSet(client, ic)...) | |||
allErrs = append(allErrs, ValidateSecurityProfileDiskEncryptionSet(client, ic)...) | |||
allErrs = append(allErrs, validateSkipImageUpload(field.NewPath("image"), ic.ControlPlane.Platform)...) |
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.
There should be a validation for compute nodes as well as the default machine platform.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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 |
I've tested now with setting compute nodes to use the image gallery and things work. I think this PR is good to go once the requested validations are added. |
Adding option to skip the image upload from the installer if an environment variable is set since it takes a lot of time and the marketplace images can be used to skip this step.
dbef392
to
f998912
Compare
/lgtm |
/retest-required |
/label acknowledge-critical-fixes-only |
@rna-afk: 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. |
/retest-required |
Adding option to skip the image upload from the installer if an environment variable is set since it takes a lot of time and the marketplace images can be used to skip this step.