Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: merge feature flags with returned values from API to enrich flag availability in config #5291

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

estephinson
Copy link
Contributor

@estephinson estephinson commented Sep 15, 2023

πŸŽ‰ Thanks for submitting a pull request! πŸŽ‰

Summary

  • Ensures feature flags that have been returned by the API are available within the context of netlify-config

For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code πŸ§‘β€πŸ’». This ensures
    we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
    something that`s on fire πŸ”₯ (e.g. incident related), you can skip this step.
  • Read the contribution guidelines πŸ“–. This ensures
    your code follows our style guide and passes our tests.
  • Update or add tests (if any source code was changed or added) πŸ§ͺ
  • Update or add documentation (if features were changed or added) πŸ“
  • Make sure the status checks below are successful βœ…

A picture of a cute animal (not mandatory, but encouraged)

@estephinson estephinson requested review from a team as code owners September 15, 2023 11:10
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

Can we add some tests?

@estephinson
Copy link
Contributor Author

Can we add some tests?

I can't see an easy path to getting tests around this.

I've refactored to move the logic into getSiteInfo, but will be adding a single unit test to assert on that

Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

I can see that asserting on the feature flags is a bit challenging given the current setup. I think it would be nice to get some tests on this area, because if we introduce a regression that stops certain feature flags from being surfaced, that can quickly become a super tricky problem to troubleshoot.

That's a bigger lift though, so it doesn't have to be part of this PR.

// Feature flags can also be loaded from the call to `getSiteInfo()`, in the CLI
// these won't be accessible within the calls to `netlify-config` as they're fetched
// too late. We merge them here to ensure they're available to this package.
const featureFlags = { ...passedFeatureFlags, ...(siteInfo.feature_flags ?? {}) }
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this fallback?

Suggested change
const featureFlags = { ...passedFeatureFlags, ...(siteInfo.feature_flags ?? {}) }
const featureFlags = { ...passedFeatureFlags, ...siteInfo.feature_flags }

@github-actions github-actions bot added the stale label Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants