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

Error mirroringK8S=true but Consul NS's disabled #695

Merged
merged 1 commit into from Nov 19, 2020
Merged

Error mirroringK8S=true but Consul NS's disabled #695

merged 1 commit into from Nov 19, 2020

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Nov 18, 2020

Based on an issue someone had reported. They had set

connectInject:
  consulNamespaces:
    mirroringK8S: true

But didn't have

global:
  enableConsulNamespaces: true

This caused us to ignore that configuration.

NOTE: We can't error on connectInject.consulNamespaces.destinationNS because that has a default value so it's always set.

@lkysow lkysow requested review from thisisnotashwin, a team and kschoche and removed request for a team November 18, 2020 23:28
thisisnotashwin
thisisnotashwin reviewed Nov 19, 2020
View reviewed changes
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.

Thanks for making this change!! Just have a suggestion with regards to the test but it looks great otherwise.

test/unit/connect-inject-deployment.bats Show resolved Hide resolved
thisisnotashwin
thisisnotashwin approved these changes Nov 19, 2020
View reviewed changes
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.

🦊

@lkysow lkysow merged commit 4308446 into master Nov 19, 2020
@lkysow lkysow deleted the error-mirroring branch November 19, 2020 18:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thisisnotashwin thisisnotashwin thisisnotashwin approved these changes

@kschoche kschoche kschoche approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lkysow @kschoche @thisisnotashwin