-
Notifications
You must be signed in to change notification settings - Fork 53
Remove: machine_accounts feature flag. #5971
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
base: main
Are you sure you want to change the base?
Remove: machine_accounts feature flag. #5971
Conversation
Signed-off-by: Mujib Ahasan <ahasanmujib8@gmail.com>
|
The test failure appears to be valid: |
| return nil, util.UserVisibleError(codes.Unimplemented, "human users may only be added by invitation") | ||
| } | ||
| if isMachine && !flags.Bool(ctx, s.featureFlags, flags.MachineAccounts) { | ||
| if isMachine { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, this is tricky, since this is a negative use of the flag -- in this case, I think flags.Bool(ctx, s.featureFlags, flags.MachineAccounts) should effectively always return true, so if isMachine && !true will always evaluate to false, and the entire if false {...} block can be removed.
|
This PR needs additional information before we can continue. It is now marked as stale because it has been open for 30 days with no activity. Please provide the necessary details to continue or it will be closed in 30 days. |
|
I think the test failure here is clear and fairly simple (the wrong flag branch was removed). |
|
This PR needs additional information before we can continue. It is now marked as stale because it has been open for 30 days with no activity. Please provide the necessary details to continue or it will be closed in 30 days. |
Summary
The default value of
machine_accountsflag is false now. But related functionalities sets it to true. This PR is raised for the release of the flag.Fixes #5435