Remove configdir from Gopkg.toml

[cuonglm]

Usages of configdir library were removed in #1162, but we forget to remove it from Gopkg.toml.

This PR removes its entry from Gopkg.toml, now dep ensure run without warnings.

[mstoykov]

isn't the whole point of the circleci's deps step to basically fail on such problems ?
I would argue that should be fixed first and than this to be cherry-picked on top and merge both of them

[codecov-io]

Codecov Report

Merging #1211 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1211      +/-   ##
==========================================
+ Coverage   73.63%   73.65%   +0.01%     
==========================================
  Files         147      147              
  Lines       10640    10640              
==========================================
+ Hits         7835     7837       +2     
+ Misses       2345     2344       -1     
+ Partials      460      459       -1

Impacted Files	Coverage Δ
core/engine.go	94.73% <0%> (+0.95%)	⬆️

---

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 90b62b3...df60118. Read the comment docs.

[cuonglm]

isn't the whole point of the circleci's deps step to basically fail on such problems ?
I would argue that should be fixed first and than this to be cherry-picked on top and merge both of them

It's just a warning, not an error. I copied the message as it will explain better than me:

Warning: the following project(s) have [[constraint]] stanzas in Gopkg.toml:

✗ github.com/shibukawa/configdir

However, these projects are not direct dependencies of the current project:
they are not imported in any .go files, nor are they in the 'required' list in
Gopkg.toml. Dep only applies [[constraint]] rules to direct dependencies, so
these rules will have no effect.

Either import/require packages from these projects so that they become direct
dependencies, or convert each [[constraint]] to an [[override]] to enforce rules
on these projects, if they happen to be transitive dependencies.

[na--]

Yeah, it's strange that this wasn't caught by the CI 😕 In any case, the PR LGTM, and I'm not sure it's worth spending a lot of time on this, given that we'd migrate to Go modules soon... Though that's also a problem there: golang/go#27348

[na--]

There's apparently a dep check command that we don't use: golang/dep#1912
@cuonglm, can you add it to https://github.com/loadimpact/k6/blob/90b62b38d6dd96b40f2e0d346230e2c19a748723/.circleci/config.yml#L18-L20

Unfortunately, it doesn't cover this particular problem, but it covers quite a lot of other ones, so it's worth having, even for a short while

[cuonglm]

There's apparently a dep check command that we don't use: golang/dep#1912
@cuonglm, can you add it to

https://github.com/loadimpact/k6/blob/90b62b38d6dd96b40f2e0d346230e2c19a748723/.circleci/config.yml#L18-L20

Unfortunately, it doesn't cover this particular problem, but it covers quite a lot of other ones, so it's worth having, even for a short while

Done.

[mstoykov]

7273e30 here is a way to ensure (pun intended) that we see warnings
https://github.com/loadimpact/k6/compare/depsFailsOnEnsureWarning see the deps for confirmation

[cuonglm]

7273e30 here is a way to ensure (pun intended) that we see warnings
https://github.com/loadimpact/k6/compare/depsFailsOnEnsureWarning see the deps for confirmation

We can do it, but just need to decide whether it's worth. Since there's no PR from you yet, I will add my comments here:

• You don't need temporary file
• Your approach fails if any dependency package named or contains warning

[mstoykov]

You can cherry-pick it or just make a new commit, I will need to make another branch just to make a PR(as the one shown has a commit to fix the problem you are fixing here) and than have a PR on which we all need to code review ... I think that will be a lot of time wasted.

Also dep ensure does NOT print anything if there are no problems in my experience .dep ensure -update --dry-run does ... I have no idea why we are currently running with --update given that we certainly aren't updating all the time and is definitely a lot slower as it would need to check if there are new versions. So I dropped it :)

[cuonglm]

You can cherry-pick it or just make a new commit, I will need to make another branch just to make a PR(as the one shown has a commit to fix the problem you are fixing here) and than have a PR on which we all need to code review ... I think that will be a lot of time wasted.

Also dep ensure does NOT print anything if there are no problems in my experience .dep ensure -update --dry-run does ... I have no idea why we are currently running with --update given that we certainly aren't updating all the time and is definitely a lot slower as it would need to check if there are new versions. So I dropped it :)

So we decide to make CI fails in case dep emit a warning? cc @na--

[mstoykov]

We might want to set pipefail as otherwise my solution won't fail if dep ensure exits with a non-zero code but doesn't output Warning anywhere

[mstoykov]

Actually looking at dep ensure cli help maybe deps should fail on ANY output from it ?

[cuonglm]

@mstoykov You can simply do:

case $(dep ensure 2>&1 | head -n 1) in Warning*) false ;; esac

[mstoykov]

case $(dep ensure 2>&1 | head -n 1) in Warning*) false ;; esac

I have 2 problems with this:

1. it won't show the output of dep ensure in the logs which will mean that we failed and there is nothing in the logs to tell us why
2. This still means that we need to use set -o pipefail in order for circleci to fail if dep ensure returns non-zero exit code. Which also means that we need to change to bash in that particular using the shell

[na--]

Ah, yeah, completely missed that... 😪 ☕ So... 👍 for @mstoykov's solution with a teed temporary file

[cuonglm]

it won't show the output of dep ensure in the logs which will mean that we failed and there is nothing in the logs to tell us why

We don't want (don't need) to see dep ensure output, the one who make the PR should run in his/her local to check the output, CI testing is for verification only.

This still means that we need to use set -o pipefail in order for circleci to fail if dep ensure returns non-zero exit code. Which also means that we need to change to bash in that particular using the shell

I don't see why we need set -o pipefail. The case returns false as long as the first line of dep ensure start with Warning.

It's not the pipe which reports error.

[cuonglm]

it won't show the output of dep ensure in the logs which will mean that we failed and there is nothing in the logs to tell us why

We don't want (don't need) to see dep ensure output, the one who make the PR should run in his/her local to check the output, CI testing is for verification only.

This still means that we need to use set -o pipefail in order for circleci to fail if dep ensure returns non-zero exit code. Which also means that we need to change to bash in that particular using the shell

I don't see why we need set -o pipefail. The case returns false as long as the first line of dep ensure start with Warning.

It's not the pipe which reports error.

FYI:

$ case $(dep ensure 2>&1 | head -n 1) in Warning*) false ;; esac || echo 'dep ensure fail'
dep ensure fail
$ git checkout cuonglm/remove-configdir-from-dep
Switched to branch 'cuonglm/remove-configdir-from-dep'
$ case $(dep ensure 2>&1 | head -n 1) in Warning*) false ;; esac || echo 'dep ensure fail'
$

[mstoykov]

$ case $(false 2>&1 | head -n 1) in Warning*) false ;; esac || echo 'dep ensure fail'

edit: this shows that if dep ensure exits with a non-zero exit code, the circleci job would not fail unless we set pipefail

[cuonglm]

To be clear,

$ case $(false 2>&1 | head -n 1) in Warning*) false ;; esac || echo 'dep ensure fail'

Can you complete the comment?

[imiric]

LGTM