Bug 2035720: [Alibaba] support internal publish strategy #5534
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
bd233
changed the title
openshift-ci
bot
added
bugzilla/severity-medium
|
@bd233: This pull request references Bugzilla bug 2035720, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
staebler
left a comment
staebler
left a comment
There was a problem hiding this comment.
Conceptually, this looks good to me. I just have a minor nit about keeping the consolidated slb_ids rather than exploding it into two separate variables.
Since we don't have e2e testing for this, I will need someone to show me a successful install
- using external publishing to make sure that this does not break existing functionality and
- using internal publishing.
/approve
| @@ -22,12 +22,16 @@ output "eip_ip" { | |||
| value = alicloud_eip_address.eip.ip_address | |||
| } | |||
|
|
|||
| output "slb_ids" { | |||
| value = [alicloud_slb_load_balancer.slb_external.id, alicloud_slb_load_balancer.slb_internal.id] | |||
There was a problem hiding this comment.
I think we can keep the same slb_ids output.
output "slb_ids" {
value = concat(alicloud_slb_load_balancer.slb_external[*].id, [alicloud_slb_load_balancer.slb_internal.id])
}
There was a problem hiding this comment.
@jianli-wei Maybe need your help to test the updated code
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: staebler 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 |
openshift-ci
bot
added
the
approved
|
FYI using the build from https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/release-openshift-origin-installer-launch-gcp-modern/1483764171014148096, no the original error any more, i.e. with 'publish: Internal', the installer could go ahead. $ yq e '.publish' work2/install-config.yaml |
bd233
force-pushed
the
fix-publish-internal
branch
from
14c6690 to
208903a
Compare
|
FYI tested with a build from https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/release-openshift-origin-installer-launch-gcp-modern/1484413155458158592, to deploy a private cluster (with cco in manual mode) can succeed. |
| @@ -11,6 +11,9 @@ locals { | |||
| ) | |||
| system_disk_size = 120 | |||
| system_disk_category = "cloud_essd" | |||
| // Because of the issue https://github.com/hashicorp/terraform/issues/12570, the consumers cannot use a dynamic list for count | |||
There was a problem hiding this comment.
I don't think that this issue applies here. The list is coming in as input to the stage, so it is not a dynamic list. Nevertheless, we can leave it as is for now, since it has been tested.
There was a problem hiding this comment.
I don't think that this issue applies here.
Yes, you are right
| @@ -15,7 +16,7 @@ data "alicloud_alidns_domains" "dns_public" { | |||
| } | |||
|
|
|||
| resource "alicloud_alidns_record" "dns_public_record" { | |||
| count = length(data.alicloud_alidns_domains.dns_public.domains) == 0 ? 0 : 1 | |||
| count = local.is_external && length(data.alicloud_alidns_domains.dns_public.domains) != 0 ? 1 : 0 | |||
There was a problem hiding this comment.
As per https://bugzilla.redhat.com/show_bug.cgi?id=2041926, we actually want this to fail when local.is_external is true and length(data.alicloud_alidns_domains.dns_public.domains is 0. In that case, the user requested external publishing but supplied a base domain for which there is no zone. Nevertheless, we can keep this as is here and address the BZ in a separate issue.
|
|
||
| output "slb_group_length" { | ||
| // 1 for private endpoints and 1 for public endpoints | ||
| value = "2" |
There was a problem hiding this comment.
Why is this hard-coded to 2 rather than using the actual length of slb_ids?
There was a problem hiding this comment.
Refer to AWS's usage of aws_lb_target_group_arns_length in cluster/vpc/output.tf.
I updated this part and the test looks ok.
bd233
force-pushed
the
fix-publish-internal
branch
2 times, most recently
from
5e61b8b to
dea49b6
Compare
openshift-ci
bot
added
the
needs-rebase
|
This needs a rebase |
bd233
force-pushed
the
fix-publish-internal
branch
from
dea49b6 to
efed2e5
Compare
openshift-ci
bot
removed
the
needs-rebase
Have done |
|
tf-fmt failure is real. Please fix. Thanks. |
Support internal publish strategy for platform Alibaba Cloud Signed-off-by: sunhui <[email protected]> å
bd233
force-pushed
the
fix-publish-internal
branch
from
efed2e5 to
d91ecac
Compare
|
Okay, I have format it |
|
/lgtm |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
11 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@bd233: The following tests failed, say
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. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@bd233: All pull requests linked via external trackers have merged: Bugzilla bug 2035720 has been moved to the MODIFIED state. 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. |
7 participants
Support internal publish strategy for platform Alibaba Cloud