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

fix #1239: slice flag value don't append to default values from ENV or file #1240

Merged
merged 2 commits into from Jul 7, 2021

Conversation

vipally
Copy link
Contributor

@vipally vipally commented Feb 8, 2021

…r file

What type of PR is this?

  • bug
  • cleanup
  • documentation
  • feature

What this PR does / why we need it:

fix v2 bug: slice flag value don't append to default values from ENV or file

Which issue(s) this PR fixes:

Fixes #1239

Special notes for your reviewer:

StringSliceFlag have been fixed this bug,
but Float64SliceFlag, Int64SliceFlag, IntSliceFlag have the same bug.

Testing

TestFlagsFromEnv has cover this case

Release Notes

fix #1239: slice flag value don't append to default values from ENV or file

@vipally vipally requested a review from a team as a code owner February 8, 2021 11:51
@vipally vipally requested review from saschagrunert and asahasrabuddhe and removed request for a team February 8, 2021 11:51
coilysiren
coilysiren previously approved these changes Feb 10, 2021
Copy link
Member

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

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

LGTM, for other people's context the pattern here is from #1078

@@ -129,6 +129,9 @@ func (f *Float64SliceFlag) Apply(set *flag.FlagSet) error {
}
}

// Set this to false so that we reset the slice if we then set values from
// flags that have already been set by the environment.
f.Value.hasBeenSet = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you need this. Did you try running the additional tests with s.hasBeenSet = true , they run just fine.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also not sure, but since this matches what we have for the other flags, I think it's a reasonable change to bring in.

@stale
Copy link

stale bot commented Jun 28, 2021

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

@stale stale bot added the status/stale stale due to the age of it's last update label Jun 28, 2021
@stale
Copy link

stale bot commented Jul 7, 2021

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

1 similar comment
@stale
Copy link

stale bot commented Jul 7, 2021

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

@stale stale bot removed the status/stale stale due to the age of it's last update label Jul 7, 2021
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.

slice flag value don't append to default values from ENV or file
4 participants