-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add support for installing pre create route53 hostedzone cluster on AWS #27656
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
Conversation
4b66749 to
faf7717
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.
Suggest to move ipi-conf-aws behind ipi-conf-aws-route53 step.
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.
Done
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.
suggest to remove this irrelevant setting
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.
Done
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 saw we did not install jq in ci-operator/step-registry/aws/deprovision/route53/aws-deprovision-route53-commands.sh, so shall we explicitly install it here?
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.
Yes, here it is required to get the hostedzone and change id from the json file.
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 meant jq should be already installed, if not, aws-deprovision-route53 ref would fail. So sounds like we do not need to install jq here.
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.
Yes, no need to install jq.
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.
sounds like we do not need this env var in this case
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.
Removed it from the file.
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.
- Personally I think we do not have to print install-config anywhere.
- Even we check the install-config.yaml files, this patch will add
proxyandadditionalTrustBundlethere, especially when install-config.yaml never set these items, this will easily confuse our debugging.
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.
Removed code to print install-config file
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.
IMO, it's better that update the name to reflect the AWS resources we are trying to create, like aws-provision-route53-private-hosted-zone (the private-hosted-zone is a subfolder of route53)
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.
Done
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.
publish will be controlled by system environment in *-provision chain, like https://github.com/openshift/release/pull/24888/files#diff-55d9cca8aaec70f1288e64a0af48754c084edcd57938c5b53ade1701ae6c4974R22-R24, so it should be removed from here.
there are some conflicts with #24888, the subnet and AZ should be set in ipi-conf-aws-custom-vpc, which is newly introduced in #24888, but it has not been merged yet, hope it can be merged within this week.
eventually, this part should be:
cat > "${CONFIG_ROUTE53_HOSTEDZONE}" << EOF
platform:
aws:
hostedZone: $(cat "${hostedzoneid}")
EOF
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.
Sure, I will update once this #24888 merged to the master.
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.
as comment in https://github.com/openshift/release/pull/27656/files#r846992212, the step ipi-conf-aws-custom-vpc will be required before ipi-conf-aws
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.
Done
6ca4563 to
073c2a8
Compare
|
/retest |
|
/retest-required |
68d6a99 to
991d31d
Compare
de01e57 to
67a0359
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.
No need to put this patch file in SHARED_DIR, is there? It's only consumed locally a few lines below. Maybe /tmp/install-config.yaml.patch?
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.
Done
wking
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
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jianlinliu, mhanss, wking, yunjiang29 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
|
@mhanss: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
@mhanss: Updated the following 3 configmaps:
DetailsIn 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. |
Add support for installing pre created route53 cluster.