Skip to content

Conversation

mkcp
Copy link
Contributor

@mkcp mkcp commented Aug 1, 2025

Description

This PR implements the internal/feature API as described in ZEP-0035 Feature Flags. It creates a new package feature which allows maintainers to set default Features, and provides facilities for users to enable and disable features for use in the SDK. Before release we should ship at least one default feature flag, even a test/documentation noop feature struct, to enforce that setDefault can only be called once.

In a follow-up PR we'll implement:

  • These same user facilities (feature.SetUser) will be applied in the cmd layer to allow users to configure their own features via CLI flags, Env vars, and on-disk config.

Related Issue

Implements part of ZEP-0035

Checklist before merging

Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp mkcp self-assigned this Aug 1, 2025
@mkcp mkcp added enhancement ✨ New feature or request go Pull requests that update Go code labels Aug 1, 2025
Copy link

netlify bot commented Aug 1, 2025

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit 74c5e42
🔍 Latest deploy log https://app.netlify.com/projects/zarf-docs/deploys/689236521f1b20000960865a

mkcp added 5 commits August 1, 2025 11:14
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Copy link

codecov bot commented Aug 1, 2025

Codecov Report

❌ Patch coverage is 94.25287% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/internal/feature/feature.go 94.25% 3 Missing and 2 partials ⚠️
Files with missing lines Coverage Δ
src/internal/feature/feature.go 94.25% <94.25%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mkcp added 3 commits August 1, 2025 12:25
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp mkcp changed the title feat(feature): Implement ZEP-0035 Feature Flags feat(feature): Implement Feature API Aug 4, 2025
@mkcp mkcp marked this pull request as ready for review August 4, 2025 17:58
@mkcp mkcp requested review from a team as code owners August 4, 2025 17:58
mkcp added 2 commits August 4, 2025 11:04
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Copy link
Contributor

@AustinAbro321 AustinAbro321 left a comment

Choose a reason for hiding this comment

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

A few nits, otherwise looks like a solid strategy


// Set takes a slice of one or many flags, inserting the features onto user-configured features. If a feature name is
// provided that is already a part of the set, then Set will return an error.
// TODO: Should we allow users to call this multiple times even if we don't allow them to overwrite features?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think dis-allowing is fine. If there's a world where we need to allow multiple sets in the future then we could change it, but this is probably the most intuitive option since there's always an error on the second attempt

Copy link
Contributor Author

@mkcp mkcp Aug 4, 2025

Choose a reason for hiding this comment

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

Yeah i'm happy with the current behavior as well.

The main edge case I could see this creating is when a user is calling the SDK in one "context" with one set of feature flags, and then needs a different set of flags for a different context later but within the same running process. The workaround is just to enable everything up front, but issues could still arise. Simple enough to ease the restriction later though.

mkcp and others added 2 commits August 4, 2025 14:36
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Co-authored-by: Austin Abro <37223396+AustinAbro321@users.noreply.github.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
@mkcp mkcp force-pushed the mkcp/feature-experiments branch from 3912dce to 652e6cf Compare August 4, 2025 21:37
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Copy link
Member

@brandtkeller brandtkeller left a comment

Choose a reason for hiding this comment

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

Minor items for context or my education. Appreciative of the tests - they were easy to step through an observe intent.

mkcp added 2 commits August 5, 2025 08:19
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Copy link
Contributor

@AustinAbro321 AustinAbro321 left a comment

Choose a reason for hiding this comment

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

LGTM

@mkcp mkcp added this pull request to the merge queue Aug 5, 2025
github-merge-queue bot pushed a commit that referenced this pull request Aug 5, 2025
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Co-authored-by: Austin Abro <37223396+AustinAbro321@users.noreply.github.com>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 5, 2025
@mkcp mkcp added this pull request to the merge queue Aug 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 6, 2025
@mkcp mkcp added this pull request to the merge queue Aug 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 6, 2025
@mkcp mkcp added this pull request to the merge queue Aug 6, 2025
Merged via the queue into main with commit 8fb22f5 Aug 6, 2025
27 checks passed
@mkcp mkcp deleted the mkcp/feature-experiments branch August 6, 2025 18:32
Ansible-man pushed a commit to Ansible-man/zarf that referenced this pull request Sep 6, 2025
Signed-off-by: Kit Patella <kit@defenseunicorns.com>
Co-authored-by: Austin Abro <37223396+AustinAbro321@users.noreply.github.com>
Signed-off-by: Cade Thomas <cadethomas23@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants