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

feat: default-required feature flag #2211

Closed
wants to merge 147 commits into from
Closed

feat: default-required feature flag #2211

wants to merge 147 commits into from

Conversation

Noxsios
Copy link
Contributor

@Noxsios Noxsios commented Jan 2, 2024

Description

Refer to adr/0025-required-optional.md.

Related Issue

Fixes #2059

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

Copy link

netlify bot commented Jan 2, 2024

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit 0368956
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/665609da2e32780008319736

Signed-off-by: razzle <[email protected]>
Signed-off-by: razzle <[email protected]>
Signed-off-by: razzle <[email protected]>
@Noxsios Noxsios added needs-docs PR Label - Docs required to merge needs-tests PR Label - Tests required to merge breaking-change labels Jan 2, 2024
Signed-off-by: razzle <[email protected]>
Signed-off-by: razzle <[email protected]>
@Noxsios

This comment was marked as resolved.

Signed-off-by: razzle <[email protected]>
Signed-off-by: razzle <[email protected]>
@Noxsios Noxsios added the uds label Jan 2, 2024
@Noxsios Noxsios changed the title Required key --> Optional key (default is optional: false) feat: required key --> optional key Jan 2, 2024
@Noxsios Noxsios marked this pull request as ready for review January 3, 2024 02:58
@Noxsios Noxsios removed the needs-tests PR Label - Tests required to merge label May 10, 2024
@Noxsios Noxsios requested a review from dgershman as a code owner May 15, 2024 17:06
Copy link
Contributor

@lucasrod16 lucasrod16 left a comment

Choose a reason for hiding this comment

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

The behavior changes seem reasonable, but I believe we should discuss the addition of the .metadata.features field to the package schema further.

Comment on lines +27 to +31
## Decision

Option 4. Introduce `.metadata.features` flags to `zarf.yaml` to allow for certain schema behavior to be configurable to the user.

The `features` key will be added to the `metadata` section of the package schema. This key will be an array of strings, where each string is the name of a beta feature that can be enabled. To enable a feature, the user will need to add the name of the feature to this array and edit the package schema accordingly.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the .metadata.features field might not be necessary for this purpose. Have we considered keeping the required key optional in the schema and defaulting to required: true for all components unless explicitly set to required: false? This would achieve the same behavior as the default-required flag but without needing the feature flag.

@Noxsios Noxsios mentioned this pull request Jun 14, 2024
2 tasks
@AustinAbro321
Copy link
Contributor

This is going to be done in the v1 schema instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

⚠ Breaking Change: make components required by default
4 participants