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

staticTargeting: add flag to switch default to true #577

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

Conversation

nathantournant
Copy link
Member

What does this PR do?

  • Adds new functionality
  • Alters existing functionality
  • Fixes a bug
  • Improves documentation or testing

Please briefly describe your changes as well as the motivation behind them:

  • Now places the default value for staticTargeting to a flag in the controller's config map.

Code Quality Checklist

  • The documentation is up to date.
  • My code is sufficiently commented and passes continuous integration checks.
  • I have signed my commit (see Contributing Docs).

Testing

  • I leveraged continuous integration testing
    • by depending on existing unit tests or end-to-end tests.
    • by adding new unit tests or end-to-end tests.
  • I manually tested the following steps:
    • Deploy the controller with the staticTargetingDefault field set to false, launch a disruption with staticTargeting unspecified, check out the description
    • Do all of those combinations (flag false/true, staticTargeting field unspecified/false/true) and assert you have the expected results.
    • locally.
    • as a canary deployment to a cluster.

@nathantournant nathantournant requested a review from a team as a code owner August 22, 2022 16:24
@nathantournant nathantournant self-assigned this Aug 22, 2022
@@ -44,7 +44,7 @@ type DisruptionSpec struct {
DryRun bool `json:"dryRun,omitempty"` // enable dry-run mode
OnInit bool `json:"onInit,omitempty"` // enable disruption on init
Unsafemode *UnsafemodeSpec `json:"unsafeMode,omitempty"` // unsafemode spec used to turn off safemode safety nets
StaticTargeting bool `json:"staticTargeting,omitempty"` // enable dynamic targeting and cluster observation
StaticTargeting *bool `json:"staticTargeting,omitempty"` // disable dynamic targeting and cluster observation
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this have to become a pointer again? The default for a bool is already false, no?

Copy link
Member Author

@nathantournant nathantournant Aug 22, 2022

Choose a reason for hiding this comment

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

afaik go bool unmarshalling doesn't make a difference between false and empty value -- if it's not a pointer.
Setting a default to true would be overridden by a missing/defaulted field that would be interpreted as a false

Copy link
Member Author

@nathantournant nathantournant Aug 22, 2022

Choose a reason for hiding this comment

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

This PR aims to allow true to be a possible default value based on the chart
This value, if set, needs to not be overridden by an empty one
edit: fixed the title, sorry about the confusion

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. There is another option, I think? At the cost of some wordier unmarshaling, we can avoid making this be a pointer everywhere.

If we make the type of StaticTargeting not bool, but a custom type: type StaticTargetingFlag bool, we can add a custom Unmarshal for that type that uses the chart-defined default when empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried a few things, hoping this would work; without a new dedicated struct type for the field -which would not really help- the UnmarshalJSON function is not called on an empty value 😞
I'm afraid the pointer is unavoidable there

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it..... too late to change this field to "Targeting" with the options of "dynamic,static,empty", where empty uses the default? Representing three states by using a pointer to a bool feels wrong

@nathantournant nathantournant changed the title staticTargeting: add flag to switch default to false staticTargeting: add flag to switch default to true Aug 22, 2022
@nathantournant nathantournant force-pushed the nathan/static-targeting-default-flag branch from 4aa861c to 53ec941 Compare August 24, 2022 14:56
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.

User Request: Release Dynamic Targeting behind a feature flag in controller
2 participants