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

Open
wants to merge 147 commits into
base: main
Choose a base branch
from
Open

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

Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
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 <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
@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 <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
@Noxsios

This comment was marked as resolved.

Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
@Noxsios Noxsios added the uds label Jan 2, 2024
Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
@Noxsios Noxsios changed the title Required key --> Optional key (default is optional: false) feat: required key --> optional key Jan 2, 2024
Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
@Noxsios Noxsios marked this pull request as ready for review January 3, 2024 02:58
Noxsios and others added 9 commits April 9, 2024 23:01
@Noxsios Noxsios removed the needs-tests PR Label - Tests required to merge label May 10, 2024
Noxsios and others added 5 commits May 10, 2024 03:19
Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
@Noxsios Noxsios requested a review from dgershman as a code owner May 15, 2024 17:06
Noxsios and others added 4 commits May 15, 2024 12:15
Signed-off-by: razzle <harry@razzle.cloud>
Signed-off-by: razzle <harry@razzle.cloud>
Copy link
Member

@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
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: New
Development

Successfully merging this pull request may close these issues.

⚠ Breaking Change: make components required by default
4 participants