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
For Azure cloud prepare get the image from existing Machineset #594
Conversation
🤖 Created branch: z_pr594/aswinsuryan/azure-image |
5c6b986
to
570e71c
Compare
Setting priority as high and marking for backport, as currently all OpenShift 4.12 Azure based deployments are blocked without this fix. |
547d18e
to
e44a051
Compare
2442946
to
00ca647
Compare
fbb487d
to
0cf928f
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.
Minor idiomatic change, but not worth respinning. I have another concern regarding GetWorkerNodeImage but it’s not specific to this PR, I’ll submit another PR for it.
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.
Generally looks good, one suggestion if youre going to respin the PR, otherwise don't bother
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 hasn’t been resolved (and I’d missed the mismatch in line 130, which means it needs to be resolved).
pkg/azure/ocpgwdeployer.go
Outdated
image, imageErr := d.msDeployer.GetWorkerNodeImage(nil, nil, d.InfraID) | ||
if imageErr != nil { | ||
return errors.Wrap(err, "error retrieving worker node image") |
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.
image, imageErr := d.msDeployer.GetWorkerNodeImage(nil, nil, d.InfraID) | |
if imageErr != nil { | |
return errors.Wrap(err, "error retrieving worker node image") | |
image, err := d.msDeployer.GetWorkerNodeImage(nil, nil, d.InfraID) | |
if err != nil { | |
return errors.Wrap(err, "error retrieving worker node image") |
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 lead to linting error
pkg/azure/ocpgwdeployer.go:133:3: ineffectual assignment to err (ineffassign)
err = d.deployDedicatedGWNode(machineSets, gatewayNodesToDeploy, image, status)
Hence reverting, Will fix the issue in line 132.
0cf928f
to
686bced
Compare
Signed-off-by: Aswin Suryanarayanan <aswinsuryan@gmail.com>
686bced
to
4ae59d3
Compare
A suggested change was reverted after approval.
image, imageErr := d.msDeployer.GetWorkerNodeImage(nil, nil, d.InfraID) | ||
if imageErr != nil { | ||
return errors.Wrap(imageErr, "error retrieving worker node image") | ||
} |
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.
image, imageErr := d.msDeployer.GetWorkerNodeImage(nil, nil, d.InfraID) | |
if imageErr != nil { | |
return errors.Wrap(imageErr, "error retrieving worker node image") | |
} | |
var image string | |
image, err = d.msDeployer.GetWorkerNodeImage(nil, nil, d.InfraID) | |
if err != nil { | |
return errors.Wrap(err, "error retrieving worker node image") | |
} |
🤖 Closed branches: [z_pr594/aswinsuryan/azure-image] |
@aswinsuryan can you take care of the 0.13 and 0.12 backports? |
It requires two other back ports to resolve conflicts, It is not required in 0.12 as Azure support is added in 0.13 |
For Azure cloud prepare it uses a hard-coded path and infra id for the machine set. Now it is changed the image. from one of the existing worker nodes.
Signed-off-by: Aswin Suryanarayanan aswinsuryan@gmail.com