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 2.1.0 regression #1014

Merged
merged 13 commits into from
Dec 25, 2019
Merged

Fix 2.1.0 regression #1014

merged 13 commits into from
Dec 25, 2019

Conversation

coilysiren
Copy link
Member

@coilysiren coilysiren commented Dec 24, 2019

Motivation

Fixes #1015

Release Notes

Fixed a Context regression introduced in #1013

Changes

When passing in context.Background(), initialize a *flagSet value. This prevents the code from panicking when flagSet is accessed later.

Testing

In addition to the automated tests, I stood up a testing package (here => coilysiren/testing-cli#1) with this new code. The testing package ran successfully with this new cli code.

Followup

After v2.1.1 is released, I'll remove the commented out documentation lines. Also, this issue creates more justification for investigating #1006.

Reviewer Guidelines

All our builds are broken until this is resolved 😅

@coilysiren coilysiren changed the title Update manual.md Fix 2.1.0 regression Dec 24, 2019
@coilysiren coilysiren self-assigned this Dec 24, 2019
@codecov
Copy link

codecov bot commented Dec 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ed1a2ab). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1014   +/-   ##
========================================
  Coverage          ?   72.5%           
========================================
  Files             ?      33           
  Lines             ?    2495           
  Branches          ?       0           
========================================
  Hits              ?    1809           
  Misses            ?     575           
  Partials          ?     111
Impacted Files Coverage Δ
context.go 95.45% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed1a2ab...5b57b2b. Read the comment docs.

@coilysiren coilysiren marked this pull request as ready for review December 24, 2019 17:46
@coilysiren coilysiren requested a review from a team as a code owner December 24, 2019 17:46
@coilysiren coilysiren requested review from rliebz, asahasrabuddhe and saschagrunert and removed request for a team December 24, 2019 17:46
@coilysiren
Copy link
Member Author

In case it'd be helpful to identify, the cause here is this diff

- context := NewContext(a, set, nil)
+ context := NewContext(a, set, &Context{Context: ctx})

via https://github.com/urfave/cli/pull/975/files#diff-a66cc59eb6bfe62b7b3b69cd52b84747L227-R235

Copy link
Member

@asahasrabuddhe asahasrabuddhe left a comment

Choose a reason for hiding this comment

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

LGTM

@coilysiren coilysiren merged commit 723f284 into master Dec 25, 2019
@coilysiren coilysiren deleted the lynncyrin-patch-12 branch December 25, 2019 16:49
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.

All builds currently failing
3 participants