Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Enable feature-flag for context-target by default #4081

Merged
merged 1 commit into from
Dec 3, 2022

Conversation

anujc25
Copy link
Contributor

@anujc25 anujc25 commented Dec 2, 2022

What this PR does / why we need it

  • This PR enables the Context/Target feature-flag by default.

  • This change requires rename of the feature-flag to context-target-v2 to make sure default value gets update in the user's config.yaml if context-target feature-flag already exists.

    • This is required because if the feature-flag already exists in config.yaml than current logic does not change the value.

Which issue(s) this PR fixes

Fixes #3653

Describe testing done for PR

  • Verified that all the Context/Target related commands/features are enabled and user can use it without configuring any additional feature-flag manually

Release note

Enable Context/Target feature by default

Additional information

Special notes for your reviewer

@anujc25 anujc25 requested a review from a team as a code owner December 2, 2022 03:33
@anujc25
Copy link
Contributor Author

anujc25 commented Dec 2, 2022

/test install-vc7

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

Works great.

LGTM

@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@1d939a0). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #4081   +/-   ##
=======================================
  Coverage        ?   47.77%           
=======================================
  Files           ?      469           
  Lines           ?    45475           
  Branches        ?        0           
=======================================
  Hits            ?    21726           
  Misses          ?    21697           
  Partials        ?     2052           
Impacted Files Coverage Δ
cli/core/pkg/config/featureflags.go 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mpanchajanya
Copy link
Contributor

LGTM.

@alfredthenarwhal
Copy link
Collaborator

@anujc25: /test install-vc7
Commit: 7b807c6

Tests passed! Build no: 3441

Copy link
Contributor

@prkalle prkalle left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@vuil
Copy link
Contributor

vuil commented Dec 2, 2022

nit: in your PR description:
current logic does change the value
I think you mean does not change the value.

Still thinking if there is a better name for the flag. using -v2 when the original was never publicly exposed feels weird.

Copy link
Contributor

@vuil vuil left a comment

Choose a reason for hiding this comment

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

lgtm. holding till other PRs get in.

@vuil vuil added area/core-cli ok-to-merge PRs should be labelled with this before merging labels Dec 2, 2022
@vuil
Copy link
Contributor

vuil commented Dec 2, 2022

given the original flag was actually out, the alternative name, while not pretty, seems reasonable.

@anujc25 anujc25 merged commit 4f4f72a into main Dec 3, 2022
@anujc25 anujc25 deleted the anujc/enable-featureflag-context-target branch December 3, 2022 01:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/core-cli cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turn on context and target functionality
8 participants