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

bugfix: don't overwrite existing stringslice refence #1178

Merged
merged 1 commit into from Sep 21, 2020

Conversation

@alexcb
Copy link

@alexcb alexcb commented Aug 24, 2020

What type of PR is this?

(REQUIRED)

  • bug
  • cleanup
  • documentation
  • feature

What this PR does / why we need it:

fixes an issue where values set by environment var are not saved to
existing stringslice reference.

Which issue(s) this PR fixes:

No issue was opened

Testing

Included a small unit test

Release Notes

(REQUIRED)

fixed stringsliceflag apply logic to reuse existing value reference when ENV value is defined
fixes an issue where values set by environment var are not saved to
existing stringslice reference.
@alexcb alexcb requested a review from urfave/cli as a code owner Aug 24, 2020
@alexcb alexcb requested review from saschagrunert and rliebz and removed request for urfave/cli Aug 24, 2020
alexcb added a commit to earthly/earthly that referenced this pull request Aug 25, 2020
current master version of urfave/cli has a bug where values set by the
environment var are not saved to the stringslice var.

Once urfave/cli#1178 is merged, we can switch
back to using the upstream version.
alexcb added a commit to earthly/earthly that referenced this pull request Aug 25, 2020
current master version of urfave/cli has a bug where values set by the
environment var are not saved to the stringslice var.

Once urfave/cli#1178 is merged, we can switch
back to using the upstream version.

Co-authored-by: Alex Couture-Beil <alex@earthly.dev>
@rliebz
rliebz approved these changes Aug 26, 2020
@alexcb
Copy link
Author

@alexcb alexcb commented Sep 21, 2020

@saschagrunert can I get a review on this?

Copy link
Member

@saschagrunert saschagrunert left a comment

Yep, LGTM

@alexcb
Copy link
Author

@alexcb alexcb commented Sep 21, 2020

thanks @saschagrunert , is there anything else that needs to be done before this can be merged?

@saschagrunert saschagrunert merged commit f539894 into urfave:master Sep 21, 2020
12 checks passed
12 checks passed
ubuntu-latest @ Go 1.12
Details
ubuntu-latest @ Go 1.13
Details
ubuntu-latest @ Go 1.14
Details
macos-latest @ Go 1.12
Details
macos-latest @ Go 1.13
Details
macos-latest @ Go 1.14
Details
windows-latest @ Go 1.12
Details
windows-latest @ Go 1.13
Details
windows-latest @ Go 1.14
Details
test-docs
Details
codecov/patch 100.00% of diff hit (target 73.37%)
Details
codecov/project 73.38% (+0.01%) compared to d2d2098
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants