Additional cleanup, more defaults for easier setup.#4923
Conversation
|
@costinm PR needs rebase |
|
After looking closer at this PR and the previous one #4706. I notice the |
|
@john-a-joyce before cluster registry gets init, we need to get a client to read configmap and secrtet, that is why initial kube client is from local server, once cluster registry is populated, local client should not be used and all subsequent calls should use one from registry. |
Codecov Report
@@ Coverage Diff @@
## master #4923 +/- ##
=======================================
+ Coverage 74% 74% +1%
=======================================
Files 307 307
Lines 25770 25861 +91
=======================================
+ Hits 18879 19024 +145
+ Misses 6128 6088 -40
+ Partials 763 749 -14
Continue to review full report at Codecov.
|
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
from linting gate: |
|
retest ci/circleci:lint |
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sdake Assign the PR to them by writing 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 |
| secretName, secretNamespace, key, err) | ||
| return &ClusterStore{}, nil | ||
| } | ||
| cs.clusters = append(cs.clusters, &cluster) |
There was a problem hiding this comment.
remove this line which is a duplicate of line 138
|
|
||
| multierror "github.com/hashicorp/go-multierror" | ||
| "go.uber.org/multierr" | ||
| "github.com/hashicorp/go-multierror" |
There was a problem hiding this comment.
lint doesn't like it, but the following seems to make it happy
multierror "github.com/hashicorp/go-multierror"
| cluster.ObjectMeta.Annotations[ClusterAccessConfigSecretNamespace] = "istio-system" | ||
| } | ||
| // by default, expect a secret with the same name as the cluster | ||
| cluster.ObjectMeta.Annotations[ClusterAccessConfigSecretNamespace] = cluster.Name |
There was a problem hiding this comment.
I think using just secret name is dangerous as it could cause unpleasant collisions. I would still suggest to use labels as an idnicator that secret carries cluster config. It does not have to be done in this PR, once it is merged I will push a change to address it.
There was a problem hiding this comment.
That's going away with the config changes.
sbezverk
left a comment
There was a problem hiding this comment.
Other than duplicate line, looks good.
|
Thanks @costinm. |
As long as secret name matches the cluster name (which I think is reasonable default) - it will work with no custom annotations.
The pilot address would confuse users, we're not supporting that mode yet.