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

Add Go modules support. #180

Closed
wants to merge 1 commit into from
Closed

Add Go modules support. #180

wants to merge 1 commit into from

Conversation

wlynch
Copy link
Member

@wlynch wlynch commented Oct 16, 2019

Changes

Adds Go modules support for triggers repo.

This follows a similar pattern as tektoncd/pipeline#1607. Vendor directory in place for now - would like to remove it, but requires further discussion.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Release Notes

n/a

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign ncskier
You can assign the PR to them by writing /assign @ncskier in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 16, 2019
@@ -0,0 +1,84 @@
module github.com/tektoncd/triggers

go 1.13
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, does this need go 1.13? I don't think our test runner works with 1.13 yet

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. This was auto generated by the go mod init. We can try bumping this down, but I suspect we might be fighting the tool to keep this out.

Any reason we can't bump the test runner to 1.13?

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like @dibyom went ahead and bumped it? tektoncd/plumbing#88 :D 🎉

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine having 1.13 in there 😉

Copy link
Collaborator

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @wlynch !

  • Would it make sense to include a documentation component to this PR as well, like something in DEVELOPMENT.md? (e.g. i dont know how to interact with this go.mod file at all)
  • What are the next steps before we can update update-deps.sh to use go modules instead of go dep, and remove the files we maintain for go dep
  • Can we totally switch over right now or should we make it so that update-deps.sh ensures that we are keeping both lists of dependencies up to date?

@@ -14,7 +14,6 @@
*.dll

# Fortran module files
*.mod
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

ooooooh i get it (for posterity: it looked like we were randomly changing the Fortran gitignore and i didnt understand why - but of course, the go module file ends with .mod so we need to stop ignoring it. (that must have been an interesting one to track down @wlynch !!)

but i really liked this cat so i dont want to delete this comment... 😇

@@ -0,0 +1,84 @@
module github.com/tektoncd/triggers

go 1.13
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like @dibyom went ahead and bumped it? tektoncd/plumbing#88 :D 🎉

@wlynch
Copy link
Member Author

wlynch commented Oct 29, 2019

  • Would it make sense to include a documentation component to this PR as well, like something in DEVELOPMENT.md? (e.g. i dont know how to interact with this go.mod file at all)

Done.

  • What are the next steps before we can update update-deps.sh to use go modules instead of go dep, and remove the files we maintain for go dep

Done.

  • Can we totally switch over right now or should we make it so that update-deps.sh ensures that we are keeping both lists of dependencies up to date?

Done.

@tekton-robot tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2019
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Nice PR. few remarks on it

  • I think you'll need to adjust the plumbing script (presubmit-test.sh) to set GO111MODULE to on to make sure it's taken into account by the CI
  • On cli we did keep the vendor folder for people who do not have go 1.13 yet and or don't want to downlead everything at first build (we have a check to make sure the vendor folder is in sync).

.gitmodules Outdated
@@ -0,0 +1,3 @@
[submodule "vendor/github.com/tektoncd/plumbing"]
Copy link
Member

Choose a reason for hiding this comment

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

Interesting choice 👼 Maybe we should do the same in experimental, cli and others.
We have this in cli to make it so that the plumbing repository gets tracked as a module too… I wonder which approach make the more sense. But we should all use the one that make more sense in the end (be it git submodules or the go mod hack 😛 )
/cc @bobcatfish

Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to be consistent in whether having vendor folder or not. We are still using vendor folder in cli.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm interesting! I think this highlights something very odd (and historical) about our plumbing repo - we were using go to vendor in bash scripts basically 🤔 (is there anything besides the bash scripts we need from plumbing?)

(My experiences with git submodules in the past were painful but I can't remember the specifics so maybe it's okay?)

I think no matter what we should work toward not sharing scripts from plumbing like this, so this is more motivation to migrate from the bash scripts to some combo of

Couple ideas:

  1. Use git submodule for now
  2. Update whatever actually needs the plumbing scripts to fetch them (probably our .sh e2e scripts)
  3. Hold off on this until we migrate away from the bash scripts

I'm partial to (2) tho it's more work but would settle for (1) (can always do 2 as a next step :))

Copy link
Member

Choose a reason for hiding this comment

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

hm interesting! I think this highlights something very odd (and historical) about our plumbing repo - we were using go to vendor in bash scripts basically thinking (is there anything besides the bash scripts we need from plumbing?)

It totally does 👼 😝

(My experiences with git submodules in the past were painful but I can't remember the specifics so maybe it's okay?)

I have the same experience, but it's been a long while since I used submodules.

I think no matter what we should work toward not sharing scripts from plumbing like this, so this is more motivation to migrate from the bash scripts to some combo of

Absolutely 😻. We should even create an issue in tektoncd/plumbing about this. I would really love having tektoncd/plumbing not required at all for the other projects.

Code (I thought python made a lot of sense but then we're going to have to start publishing python packages? maybe sticking to Go makes more sense)

Right, using go, this would mean all our scripts (from plumbing) should be rewritten into go and some changes would be required in the way we build / run our CI (which is ok). Python may be fine by me too ; I know @chmouel loves bash though 😛

Images

Not sure if I am thinking the same as you, but : having each repository build their own test-runner image using a base that comes from tektoncd/plumbing which would hold the shared scripts / tooling ?

Couple ideas:

  1. Use git submodule for now
  2. Update whatever actually needs the plumbing scripts to fetch them (probably our .sh e2e scripts)
  3. Hold off on this until we migrate away from the bash scripts

I'm partial to (2) tho it's more work but would settle for (1) (can always do 2 as a next step :))

I am fine going for git submodule for now (and do that in the other tektoncd repositories too)

/cc @afrittoli

Copy link
Member

Choose a reason for hiding this comment

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

dunno about bash or python (althought I rather do the later than the former) but I usually just black-out everytime there is a mention of go and modules or even just go and managing dependencies...

@bobcatfish
Copy link
Collaborator

On cli we did keep the vendor folder for people who do not have go 1.13 yet and or don't want to downlead everything at first build (we have a check to make sure the vendor folder is in sync).

Do you have a plan for how long you want to do that? I'm almost tempted to go the other way and immediately not support go 1.13 until someone requests otherwise (maybe you have use cases already @vdemeester ?)

Another option, maybe we could provide some kinda script or something that'll generate the vendor folder for folks who need it?

@vdemeester
Copy link
Member

Do you have a plan for how long you want to do that? I'm almost tempted to go the other way and immediately not support go 1.13 until someone requests otherwise (maybe you have use cases already @vdemeester ?)

On cli, the main thing was on packaging it in distribution that may lag a bit in terms of go version and/or to not have to have a crazy dependency hell to manage as part of the distribution package. That said, I think we are more or less fine, and we wanted to remove the vendor folder at some point.

Another option, maybe we could provide some kinda script or something that'll generate the vendor folder for folks who need it?

Well, it's just GO111MODULE=on go get ./... or having something that runs it in a container 😛 .

@wlynch
Copy link
Member Author

wlynch commented Oct 30, 2019

/hold

Putting this on hold to figure out the vendoring of tektoncd/plumbing.

As @bobcatfish mentioned, the only usage of the vendor directory for this repo is vendoring for the bash scripts for testing, as well as some complications around LICENSE checks. You can go build ./... and go test ./... this repo via go modules with without the vendor directory, which is great!

I'll see what I can do with the presubmit scripts. If it proves to be too difficult I'll bring back the vendor directory, with -mod=vendor

@tekton-robot
Copy link

tekton-robot commented Dec 26, 2019

@wlynch: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-tekton-triggers-build-tests 01ddc9a link /test pull-tekton-triggers-build-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dibyom dibyom added this to In progress in Triggers Apr 14, 2020
@vdemeester vdemeester mentioned this pull request Apr 24, 2020
3 tasks
Triggers automation moved this from In progress to Done Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Triggers
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants