-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[OSDOCS-4353]: Adds GCP workload ID upgrade procedure #51752
[OSDOCS-4353]: Adds GCP workload ID upgrade procedure #51752
Conversation
🤖 Updated build preview is available at: Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/2088 |
@abutcher PTAL when you have time |
71776c3
to
1808342
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.
Changes look good to me 👍
@jianping-shu PTAL 🙏 |
@abutcher @jeana-redhat There was the following error for co image-registry scenario 2, run ccoctl gcp create-all with the same output-dir in install/upgrade steps, the RSA key pair was reused and tls/bound-service-account-signing-key.key is re-generated but same I guess tls/bound-service-account-signing-key.key is used in the created resources by ccoctl, so if it re-generated then it will not match with the one in the cluster. |
@jeana-redhat Here are some comments not related to this PR. Maybe we can create another ticket for tracking them if they are valid and need some effort.
|
Yes, I hadn't considered the output-dir not being around which seems very likely. |
Thanks for confirmation! Let's change to create-service-accounts and the procedure will be fine. |
Thanks for the review! Leaving some comments below:
I actually did this a bit more granularly in a different
Yes, I have a card to make this change in 4.12: OSDOCS-4159
I opened AWS and GCP docs cards at the same time, but the GCP update is waiting on CCO-197.
Good catch! I will fix that while I am in here rather than adding to my to-do list for later 🙂 |
a5afffc
to
bac7d04
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.
Updates look good to me.
Looks good to me, LGTM |
/label peer-review-in-progress |
/remove-label peer-review-needed |
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 is really excellent work, Jeana. I've definitely learned a thing or two by reviewing this. Nice work with the ifdefs! 🚀
/remove-label peer-review-in-progress |
bac7d04
to
cef4c23
Compare
Force pushed to force rebuild before merging, no change. |
/cherrypick enterprise-4.12 |
/cherrypick enterprise-4.11 |
@jeana-redhat: new pull request created: #51878 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. |
@jeana-redhat: new pull request created: #51879 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. |
Version(s):
4.11+
Issue:
OSDOCS-4353
Link to docs preview:
QE review:
Additional information:
N/A