Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Allow unstructed schema for config in ProxyDefaults #921

Merged
merged 4 commits into from Apr 20, 2021

Conversation

ishustava
Copy link
Member

@ishustava ishustava commented Apr 20, 2021

Please see hashicorp/consul-k8s#495 for details about why this change is needed

Changes proposed in this PR:

  • Update CRD schema for ProxyDefaults based on new generated code from the above consul-k8s PR
  • Update acceptance test for proxy defaults to set all possible fields. For the config field, we're not setting all acceptable values but I think it's OK since we're trying to check that at least one is accepted by Consul.

How I've tested this PR:

  • ran acceptance tests

How I expect reviewers to test this PR:

  • code review

Checklist:

  • Bats tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@ishustava ishustava added bug Something isn't working theme/crds labels Apr 20, 2021
@ishustava ishustava changed the title Allow unstructed scheme for config in ProxyDefaults Allow unstructed schema for config in ProxyDefaults Apr 20, 2021
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

It looks like such a harmless change 😂 Nicely done!!

@ishustava ishustava requested review from a team, lkysow and ndhanushkodi and removed request for a team and lkysow April 20, 2021 21:57
@@ -92,6 +92,10 @@ func TestController(t *testing.T) {
proxyDefaultEntry, ok := entry.(*api.ProxyConfigEntry)
require.True(r, ok, "could not cast to ProxyConfigEntry")
require.Equal(r, api.MeshGatewayModeLocal, proxyDefaultEntry.MeshGateway.Mode)
require.Equal(r, "tcp", proxyDefaultEntry.Config["protocol"])
Copy link
Member

Choose a reason for hiding this comment

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

let's also test foo and member for completeness. Or we can remove those from the fixture. I like testing multiple types, so maybe bool, array, map, string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, good call. I think I've made an assumption that these are going to be ignored by consul, but they're not!

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

@ishustava ishustava requested a review from lkysow April 20, 2021 22:49
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Nice investigation and catch! Looks great!!

@ishustava ishustava merged commit 34b35d4 into master Apr 20, 2021
@ishustava ishustava deleted the allow-unstructured-schema branch April 20, 2021 23:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working theme/crds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants