-
Notifications
You must be signed in to change notification settings - Fork 119
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
ACI-CNI 5.2.3.2 upgrade #922
Conversation
siva-muni
commented
Jun 23, 2022
•
edited
Loading
edited
- Add ACI template
- Add ACI CNI template and images.
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.
@siva-muni, thank you for submitting this PR.
I've left a few specific comments to address.
Additionally, here are couple general comments:
- The build is failing, please look into why and address: https://github.com/rancher/kontainer-driver-metadata/pull/889/commits
- To merge this PR, we want it to have 2 commits - actual changes and
go generate
(so updates todata.json
) separately.
snasovich, Thanks for the review, we will address your comments. |
With these changes, we are seeing the issues, We will fix and update |
Made few more changes, testing in progress. |
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 see most of my previous comments are addressed, thank you.
Added some more comments to address.
Also, note that per #922 (comment) the best option would be to rebase after PR #921 is merged (should be very soon) to update versions it added with new ACI images /templates instead of introducing new ones. It should be merged very soon, likely today.
@siva-muni , #921 is now merged so please go ahead with rebasing this PR and updating versions added by #921 instead of introducing new ones. |
snasovich, thanks for the review, I will address the comments |
c0bae4e
to
7e7eabb
Compare
Addressed the comments, We have updated the images in the versions added by #921
|
With these changes, testing is in-progress |
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.
@siva-muni , changes LGTM, thank you for addressing my comments.
Please report the results of your testing and once you confirm it's successful and we have one more approval, this can be merged.
cc: @kinarashah
snasovich, We are finished our testing, it was successful. We are not seeing any issues. |
@siva-muni Is there any reason we don't want the images to be updated for 1.23.x and 1.24.x? If users upgrade from 1.22.x to 1.23.x aci version would downgrade, is that expected? Apologies for missing necessary context if it's obvious. |
fad301b
to
bc7efbf
Compare
kinarashah, we added the new images now for 1.23 and 1.24 versions as well. This is an oversight from our side as we are doing this for the first time. |
@siva-muni , thank you for following up on this. Are we waiting for additional testing or good to merge once @kinarashah approves? |
snasovich, We are done with testing. We are good to merge. |
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.
@siva-muni , thank you for updating the PR and reporting that the testing is successful. LGTM.
@kinarashah , could you please take a peek at it as well and if it looks good to you, please merge it.