Give better error if config entry already created #420
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lkysow
force-pushed
the
crd-migration
branch
2 times, most recently
from
e3a313f
to
3201ba5
Compare
lkysow
force-pushed
the
crd-better-error-other-dc
branch
from
e77e247
to
aa22088
Compare
Previously if the config entry already existed in Consul we'd give an error like: config entry managed in different datacenter: "other-dc" This worked well if the config entry had been created by a controller running in another Kube cluster, but if the user had created the config entry directly in Consul then it wouldn't have the datacenter annotation and so the error would actually read: config entry managed in different datacenter: "" Which was confusing. This change changes the error in that case to read: config entry already exists in Consul
lkysow
force-pushed
the
crd-better-error-other-dc
branch
from
aa22088
to
fc7fbf7
Compare
lkysow
commented
Jan 25, 2021
| @@ -1843,292 +1843,36 @@ func TestConfigEntryControllers_doesNotCreateUnownedConfigEntry(t *testing.T) { | |||
| kubeNS := "default" | |||
|
|
|||
| cases := []struct { | |||
| kubeKind string | |||
There was a problem hiding this comment.
I removed the cases for each config entry and instead added cases based on the value of the source-datacenter metadata since now we expect a different error message if it's empty vs set to something else.
I think it's okay to remove the per-config-entry cases because
- they were testing the same underlying code
- I would need to add config entry x cases permutations to test this new functionality which I think is going to make the tests too complicated
- each config entry has unit tests to ensure it implements the ConfigEntry interface properly (which is all we require to be working here)
There was a problem hiding this comment.
Can we add an Asana task to clean up the other tests as well when appropriate? Unless youve done that already :)
lkysow
requested review from
a team and
kschoche
and removed request for
a team
thisisnotashwin
approved these changes
Jan 25, 2021
Comment on lines
+350
to
+353
| if sourceDatacenter == "" { | ||
| return fmt.Errorf("config entry already exists in Consul") | ||
| } | ||
| return fmt.Errorf("config entry managed in different datacenter: %q", sourceDatacenter) |
| @@ -1843,292 +1843,36 @@ func TestConfigEntryControllers_doesNotCreateUnownedConfigEntry(t *testing.T) { | |||
| kubeNS := "default" | |||
|
|
|||
| cases := []struct { | |||
| kubeKind string | |||
There was a problem hiding this comment.
Can we add an Asana task to clean up the other tests as well when appropriate? Unless youve done that already :)
ndhanushkodi
pushed a commit
to ndhanushkodi/consul-k8s
that referenced
this pull request
Jul 9, 2021
* server-acl-init-job sets server addresses if 'externalServers.enabled' is true * server-acl-init and server-acl-init-cleanup jobs and their related resources now run either when servers are enabled or when externalServers are enabled * Add new acls.bootstrapToken value for providing your own bootstrap token. * Allow custom auth method configuration * Fail if both server and externalServers are enabled
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously if the config entry already existed in Consul we'd give an
error like:
This worked well if the config entry had been created by a controller
running in another Kube cluster, but if the user had created the config
entry directly in Consul then it wouldn't have the datacenter annotation
and so the error would actually read:
Which was confusing.
This change changes the error in that case to read:
How I've tested this PR:
How I expect reviewers to test this PR:
Checklist: