Skip to content
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

configobservation/etcd: add exception for etcd-bootstrap to not use FQDN #673

Merged

Conversation

alaypatel07
Copy link
Contributor

@alaypatel07 alaypatel07 commented Nov 26, 2019

This PR checks if the etcd-bootstrap host is configured with a real IP address
and uses it to connect to etcd instead of etcd-bootstrap FQDN. This will
relax the requirement of configuring A records in the installer for etcd-bootstrap

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 26, 2019
@deads2k
Copy link
Contributor

deads2k commented Nov 26, 2019

needs test

/assign @hexfusion

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 27, 2019
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 27, 2019
@alaypatel07
Copy link
Contributor Author

alaypatel07 commented Nov 27, 2019

secret/support created
set -o pipefail; go test -race -v -timeout 1h -p 1 -json ./test/e2e/... | gotest2junit > /tmp/artifacts/unit_report.xml
FAIL: github.com/openshift/cluster-kube-apiserver-operator/test/e2e TestEncryptionTypeAESCBC 4m0.783s
make: *** [test-unit] Error 1
---

I do not understand the error, going to try again to see if it resurfaces

/test e2e-aws-operator

@alaypatel07
Copy link
Contributor Author

/test e2e-aws-operator

Copy link
Contributor

@hexfusion hexfusion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this @alaypatel07 few comments.

pkg/operator/configobservation/etcd/observe_etcd.go Outdated Show resolved Hide resolved
vendor/modules.txt Outdated Show resolved Hide resolved
@hexfusion
Copy link
Contributor

No change to go.mod or go sum here? Can we verify the dep changes?

@alaypatel07
Copy link
Contributor Author

alaypatel07 commented Nov 27, 2019

@hexfusion since the deps were sub packages of client-go and we already have it in go.mod[1] and go.sum IMO go mod vendor just downloaded those and I committed them. The ci/prow/verify test passes which means we are good with deps?

1.

k8s.io/client-go v0.0.0

@alaypatel07
Copy link
Contributor Author

@hexfusion sure I can add the test cases. In general I assumed that ip addresses would be good because we have it from the installer and hence tested only the change in this PR

@hexfusion
Copy link
Contributor

@hexfusion sure I can add the test cases. In general I assumed that ip addresses would be good because we have it from the installer and hence tested only the change in this PR

I don't disagree but being overly defensive here isn't a bad thing.

@alaypatel07
Copy link
Contributor Author

@hexfusion addressed all concerns, PTAL

@hexfusion
Copy link
Contributor

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2019
@alaypatel07
Copy link
Contributor Author

/test e2e-aws

@alaypatel07
Copy link
Contributor Author

infra flake

level=fatal msg="failed to fetch Cluster: failed to generate asset \"Cluster\": failed to create cluster: failed to apply using Terraform"

/test e2e-aws

@alaypatel07
Copy link
Contributor Author

/test e2e-aws

Marketplace flake

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2019
vendor/modules.txt Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 2, 2019
@alaypatel07
Copy link
Contributor Author

@hexfusion I addressed the deps concern. FTR, I did a go mod tidy and go mod vendor initially that brought the other dependencies in. I reset the head to HEAD~1 and just used the go mod vendor command and that fixed the concerns. PTAL

@sttts addressed your concerns as well PTAL

@alaypatel07
Copy link
Contributor Author

Infra flake

evel=fatal msg="failed to fetch Cluster: failed to generate asset \"Cluster\": failed to create cluster: failed to apply using Terraform"

/test e2e-aws

@hexfusion
Copy link
Contributor

thanks @alaypatel07

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2019
@sttts
Copy link
Contributor

sttts commented Dec 3, 2019

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alaypatel07, hexfusion, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2019
@hexfusion
Copy link
Contributor

/retest

@openshift-merge-robot openshift-merge-robot merged commit 1454371 into openshift:master Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants