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

chore: Migrate from dep to go modules #331

Merged
merged 10 commits into from
Feb 27, 2020

Conversation

lcostea
Copy link
Member

@lcostea lcostea commented Dec 15, 2019

Migrate from dep to go modules:

  • Removed the files used by dep to install dependencies
  • Moved to using go.mod
  • Updated documentation
  • Updated ci jobs to stop using dep

@lcostea lcostea changed the title Migrate from dep to go modules chore: Migrate from dep to go modules Dec 15, 2019
@lcostea lcostea marked this pull request as ready for review December 15, 2019 14:15
Copy link
Member

@dthomson25 dthomson25 left a comment

Choose a reason for hiding this comment

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

This PR is great, but the code-gen script we use depended on the vendor folder no longer works with this change. When you run make codegen you get the following error:

mockery -dir ./metricproviders -name Provider -output ./metricproviders/mocks
Generating mock for: Provider in file: metricproviders/mocks/Provider.go
./hack/update-codegen.sh
./hack/update-codegen.sh: line 7: ../code-generator/generate-groups.sh: No such file or directory
make: *** [codegen] Error 1

To get around this, we could follow Linkerd's approach: https://github.com/linkerd/linkerd2/blob/master/bin/update-codegen.sh but I don't like how that still requires vendoring

@lcostea
Copy link
Member Author

lcostea commented Dec 17, 2019

@dthomson25 Thanks, I started looking at other projects on how they handle the code-gen and mod vendor is used by all, I couldn't see an alternative. So I will give that a try.

@dthomson25
Copy link
Member

Hey @lcostea, I wanted to check in to see if you had a chance to try to make those changes. If not worries! I know the end of the year can get busy at times!

@lcostea
Copy link
Member Author

lcostea commented Jan 8, 2020

@dthomson25 Just got back today from the New Year's vacation, I will get back on track with everything in the following days.

@lcostea lcostea force-pushed the liviu/migrate_to_go_modules branch from d02d0be to e30ea17 Compare January 10, 2020 08:08
tools.go Outdated Show resolved Hide resolved
@lcostea
Copy link
Member Author

lcostea commented Jan 10, 2020

@dthomson25 Re the codegen I found a solution that doesn't need vendoring, inspired from https://github.com/weaveworks/flagger/blob/master/hack/update-codegen.sh

@dthomson25
Copy link
Member

Awesome! That's great that we don't need to vendor the folder!

@lcostea lcostea force-pushed the liviu/migrate_to_go_modules branch from 186ffd7 to ebcaf65 Compare January 22, 2020 16:16
@lcostea lcostea requested a review from dthomson25 January 22, 2020 16:21
@lcostea
Copy link
Member Author

lcostea commented Jan 22, 2020

@dthomson25 only now I saw that all my CircleCI pipelines have been failing.
Let me work on fixing those first, no need for any review yet

@dthomson25
Copy link
Member

I took a look at your failed pipeline and I think the Prometheus library is getting upgraded and that version introduces a new method to the API interface which the mock doesn't implement.

@lcostea lcostea force-pushed the liviu/migrate_to_go_modules branch 3 times, most recently from 3f9cc07 to 7396f6a Compare January 27, 2020 18:00
@lcostea
Copy link
Member Author

lcostea commented Jan 27, 2020

Thanks, passed that step by installing golangci-lint binary directly and not as a dependency. That go get was modifying the go.mod, while before with dep it didn't have any effect.
It looks ok now, I still have failures on modules installs and builds, but it looks like an OOM issue: I get a lot of /usr/local/go/pkg/tool/linux_amd64/link: signal: killed and locally it passes when I run the same commands in the circleci container.
So it looks ready to be reviewed

@codecov
Copy link

codecov bot commented Jan 28, 2020

Codecov Report

Merging #331 into master will decrease coverage by 0.34%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #331      +/-   ##
=========================================
- Coverage   84.04%   83.7%   -0.35%     
=========================================
  Files          70      70              
  Lines        6589    6555      -34     
=========================================
- Hits         5538    5487      -51     
- Misses        762     778      +16     
- Partials      289     290       +1
Impacted Files Coverage Δ
utils/template/template.go 93.1% <0%> (-6.9%) ⬇️
rollout/bluegreen.go 76.95% <0%> (-5.33%) ⬇️
pkg/kubectl-argo-rollouts/info/replicaset_info.go 79.41% <0%> (-1.42%) ⬇️
experiments/experiment.go 93.3% <0%> (-0.34%) ⬇️
utils/experiment/experiment.go 93.04% <0%> (-0.29%) ⬇️
pkg/kubectl-argo-rollouts/cmd/get/get_rollout.go 83.76% <0%> (-0.21%) ⬇️
rollout/experiment.go 86.82% <0%> (-0.08%) ⬇️
rollout/pause.go 100% <0%> (ø) ⬆️
pkg/kubectl-argo-rollouts/info/info.go 100% <0%> (ø) ⬆️
experiments/replicaset.go 83.87% <0%> (+0.28%) ⬆️
... and 1 more

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 4bf6fa3...9c00e31. Read the comment docs.

@dthomson25
Copy link
Member

Hey @lcostea, I pushed your branch to the main repo so the code could take advantage of the increased resources. Looks like it passed!

I noticed two issues when I ran through the Makefile:

  1. I got line 10: realpath: command not found when I ran ./hack/update-codegen.sh. When I added the first answer from this stackoverflow, I was able to get it to work.

  2. I think the version of the open-api gen is different as I see

modified:   pkg/apis/api-rules/violation_exceptions.list
modified:   pkg/apis/rollouts/v1alpha1/openapi_generated.go

Do you have this behavior too?

@dthomson25
Copy link
Member

Hey @lcostea, I talked with @jessesuen, and we decided that we will move forward to the new openapi gen version and address the changes in another PR. So, we can merge this once we get the realpath fix in and fix this last merge conflict.

Install golangci-lint binary directly, not as a dependency
@lcostea lcostea force-pushed the liviu/migrate_to_go_modules branch from 7396f6a to 2ecab8d Compare February 9, 2020 18:31
@lcostea
Copy link
Member Author

lcostea commented Feb 9, 2020

@dthomson25 fixed the conflict
regarding realpath I see the command is available on the ci and I also checked alpine, debian, ubuntu, centos images and they all have it
On mac I see it is available on coreutils formula, so what about if I add this to the docs: you need to install coreutils with brew? I wouldn't want to use a dirty replacement, when the original one works on all major distros. Otherwise I will look for a standard replacement.

@jessesuen
Copy link
Member

I wouldn't want to use a dirty replacement, when the original one works on all major distros. Otherwise I will look for a standard replacement.

DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"

Is a pretty standard bash trick to get the real directory of a script -- insofar as I wouldn't even call it that dirty. I prefer using something like this which is platform agnostic, versus increasing the barrier to contribution.

@lcostea
Copy link
Member Author

lcostea commented Feb 12, 2020

@jessesuen @dthomson25 using now what you suggested for getting the script directory

Copy link
Member

@dthomson25 dthomson25 left a comment

Choose a reason for hiding this comment

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

LGTM!

Unless you want to make any other changes, I'll merge it in!

@lcostea
Copy link
Member Author

lcostea commented Feb 13, 2020

no other changes, please go ahead

@dthomson25 dthomson25 merged commit aa1dc84 into argoproj:master Feb 27, 2020
This was referenced Feb 27, 2020
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.

4 participants