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

Replace the external configdir library with the new os.UserConfigDir() #1162

Merged
merged 3 commits into from Sep 25, 2019

Conversation

cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Sep 17, 2019

This fixes #940, but adds a reminder to come back and clean up the Go 1.12 code when Go 1.14 comes out (#1165)

@codecov-io
Copy link

codecov-io commented Sep 17, 2019

Codecov Report

Merging #1162 into master will decrease coverage by 0.02%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1162      +/-   ##
==========================================
- Coverage   73.61%   73.59%   -0.03%     
==========================================
  Files         144      145       +1     
  Lines       10529    10536       +7     
==========================================
+ Hits         7751     7754       +3     
- Misses       2321     2323       +2     
- Partials      457      459       +2
Impacted Files Coverage Δ
cmd/configdir_go113.go 100% <100%> (ø)
cmd/root.go 36.36% <77.77%> (+2.36%) ⬆️
core/engine.go 92.99% <0%> (-0.94%) ⬇️

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 ab970ca...c6997f2. Read the comment docs.

mstoykov
mstoykov previously approved these changes Sep 18, 2019
Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM

cmd/configdir_go112.go Outdated Show resolved Hide resolved
imiric
imiric previously approved these changes Sep 18, 2019
cmd/root.go Show resolved Hide resolved
@cuonglm cuonglm added this to the v0.26.0 milestone Sep 24, 2019
@cuonglm cuonglm merged commit 19c6796 into master Sep 25, 2019
@cuonglm cuonglm deleted the fix/940 branch September 25, 2019 07:03
@na-- na-- changed the title Fix/940 Replace the external configdir library with the new os.UserConfigDir() Sep 25, 2019
cuonglm added a commit that referenced this pull request Sep 26, 2019
In #1162 use new os.UserConfigDir to get user config directory, and it will
panic when encounter an error. This is a backward in-compatible behavior and
can lead to bad UX.

This PR restore previous #1162 behavior. Instead of panic, we return a
default value for config dir if we can figure out user config directory.

Close #1176
cuonglm added a commit that referenced this pull request Sep 26, 2019
In #1162 use new os.UserConfigDir to get user config directory, and it will
panic when encounter an error. This is a backward in-compatible behavior and
can lead to bad UX.

This PR restore previous #1162 behavior. Instead of panic, we return a
default value for config dir if we can figure out user config directory.

Close #1176
cuonglm added a commit that referenced this pull request Sep 26, 2019
In #1162 use new os.UserConfigDir to get user config directory, and it will
panic when encounter an error. This is a backward in-compatible behavior and
can lead to bad UX.

This PR restore previous #1162 behavior. Instead of panic, we return a
default value for config dir if we can figure out user config directory.

Close #1176
srguglielmo pushed a commit to srguglielmo/k6 that referenced this pull request Nov 3, 2019
In grafana#1162 use new os.UserConfigDir to get user config directory, and it will
panic when encounter an error. This is a backward in-compatible behavior and
can lead to bad UX.

This PR restore previous grafana#1162 behavior. Instead of panic, we return a
default value for config dir if we can figure out user config directory.

Close grafana#1176
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.

Remove or replace the configdir library
4 participants