Skip to content

Comments

fix typo of feature flag everywhere. Is this an API breaking change?#2562

Closed
jschaul wants to merge 1 commit intodevelopfrom
feature-flag-typo-galore
Closed

fix typo of feature flag everywhere. Is this an API breaking change?#2562
jschaul wants to merge 1 commit intodevelopfrom
feature-flag-typo-galore

Conversation

@jschaul
Copy link
Member

@jschaul jschaul commented Jul 14, 2022

It looks like settings for the feature flag in helm chart never had any effects, as the parser looked for the version with captital E, not lowercase e.

Would one of you reviewers be able to amend/push-to/merge/close-unmerged this PR as needed? I don't have any context on what this is about; we just did diffing on configurations and saw this difference between old (hegemony) and new (helm based) prod configs.

@jschaul jschaul requested review from fisx and smatting July 14, 2022 13:16
@jschaul jschaul temporarily deployed to cachix July 14, 2022 13:16 Inactive
@jschaul jschaul marked this pull request as ready for review July 14, 2022 13:16
@elland
Copy link
Contributor

elland commented Jul 14, 2022

Nice catch. Sadly now there's a conflict.

Also I wouldn't call this an API change, if it wasn't working before.

Copy link
Contributor

@battermann battermann left a comment

Choose a reason for hiding this comment

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

Do we need to make any changes in the cailleach repo, too?

isEmailValidationEnabledTeam :: (HasCallStack, MonadSparToGalley m) => TeamId -> m Bool
isEmailValidationEnabledTeam tid = do
resp <- call $ method GET . paths ["i", "teams", toByteString' tid, "features", "validateSAMLemails"]
resp <- call $ method GET . paths ["i", "teams", toByteString' tid, "features", "validateSAMLEmails"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resp <- call $ method GET . paths ["i", "teams", toByteString' tid, "features", "validateSAMLEmails"]
resp <- call $ method GET . paths ["i", "teams", toByteString' tid, "features", featureNameBS @ValidateSAMLEmailsConfig]


instance IsFeatureConfig ValidateSAMLEmailsConfig where
type FeatureSymbol ValidateSAMLEmailsConfig = "validateSAMLemails"
type FeatureSymbol ValidateSAMLEmailsConfig = "validateSAMLEmails"
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a breaking change because URL should be case-sensitive, however in practice they might not be.

@jschaul
Copy link
Member Author

jschaul commented Sep 5, 2022

The helm chart part was fixed in #2563, see https://github.com/wireapp/wire-server/pull/2563/files#diff-252a8cec589e29d129e79ecfe2e881b1eba870106c2b1cffa618dc19b19dd54eR82-R84

The Haskell code is and remains inconsistent, but perhaps we can live with that. Someone working normally on this code can decide to follow up on this; or not.

@jschaul jschaul closed this Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants