Skip to content

Migrate to Go modules#367

Closed
honza wants to merge 10 commits into
metal3-io:masterfrom
honza:go-modules
Closed

Migrate to Go modules#367
honza wants to merge 10 commits into
metal3-io:masterfrom
honza:go-modules

Conversation

@honza
Copy link
Copy Markdown
Member

@honza honza commented Dec 16, 2019

Steps taken:

  • Run dep ensure to make sure that the vendor directory is up-to-date
  • Run go mod init ... to create Go module files
  • Run tests, build, etc to make sure everything works
  • Upgrade local operator-sdk cli tool to the latest version
  • Update requires by hand in go.mod
  • Tests, lint, etc all pass
  • Remove vendor directory
  • Remove references to dep tool from Makefile

Official migrating to Go modules documentation

@metal3-io-bot metal3-io-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 16, 2019
@zaneb
Copy link
Copy Markdown
Member

zaneb commented Dec 19, 2019

What's the thinking behind re-adding the vendor directory? From what I read this is only for compatibility with non-module-aware versions of Go?

@honza
Copy link
Copy Markdown
Member Author

honza commented Dec 23, 2019

What's the thinking behind re-adding the vendor directory?

The idea is that CI doesn't have to download all the module files.

@zaneb
Copy link
Copy Markdown
Member

zaneb commented Jan 13, 2020

OK, that sounds reasonable.
/lgtm
/test-v1a1-integration

@metal3-io-bot metal3-io-bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 13, 2020
@zaneb
Copy link
Copy Markdown
Member

zaneb commented Jan 15, 2020

/test-centos-integration

@zaneb
Copy link
Copy Markdown
Member

zaneb commented Jan 15, 2020

/test-centos-integration

1 similar comment
@zaneb
Copy link
Copy Markdown
Member

zaneb commented Jan 17, 2020

/test-centos-integration

@zaneb
Copy link
Copy Markdown
Member

zaneb commented Jan 21, 2020

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2020
@metal3-io-bot
Copy link
Copy Markdown
Contributor

New changes are detected. LGTM label has been removed.

@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 3, 2020

Updated to use operator-sdk 0.15.1.

Currently blocked by metal3-io/project-infra#50

@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 3, 2020

More context:

The current Prow job uses Go 1.12; this version doesn't automatically detect go modules. The next version, 1.13 does. All jobs which use the go binary will fail because it can't find the necessary dependencies (it's blind to the go.mod and go.sum file). With 1.13, the go command will see the go.mod file, and download the necessary packages.

We are migrating to use go modules because the operator-sdk forces us to.

@zaneb
Copy link
Copy Markdown
Member

zaneb commented Feb 3, 2020

The current Prow job uses Go 1.12; this version doesn't automatically detect go modules.

Surely that's just a matter of setting the environment variable GO111MODULE=on?

@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 3, 2020

Surely that's just a matter of setting the environment variable GO111MODULE=on?

Yes.

But 1.13.x is the latest stable?

@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 3, 2020

It seems that the canned jobs don't use our Makefile so I'm not sure how I can set the envvar in them.

@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 19, 2020

/retest

1 similar comment
@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 20, 2020

/retest

@stbenjam
Copy link
Copy Markdown
Member

/test-integration

@dhellmann dhellmann mentioned this pull request Feb 21, 2020
@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 26, 2020

/test-integration

@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 26, 2020

/test-integration

@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 26, 2020

/test gofmt

@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 26, 2020

/test-integration

@stbenjam
Copy link
Copy Markdown
Member

/approve

@metal3-io-bot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: honza, stbenjam

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

The pull request process is described here

Details 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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2020
@zaneb
Copy link
Copy Markdown
Member

zaneb commented Feb 27, 2020

This looks good.
I guess the ideal in a situation like this would be to separate out the change from dep->go mod from any version changes in dependencies. It seems like that wasn't possible in this case, and I don't want to ask you to go back and start again because it's hard enough to get anything passing all of CI!
In lieu of that, can I suggest that we document somewhere all of the dependencies that actually changed? golang and the operator-sdk seem to be two of them.
Are there any changes to checked-in generated code resulting from updating the operator-sdk?

This was referenced Feb 27, 2020
@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 27, 2020

and I don't want to ask you to go back and start again

I am a git wizard 😎 I can move some commits around.

I tried to make everything explicit in #444

  • Downgrade operator-sdk to 0.11
  • Migrate to go modules
  • Remove vendor and Gopkg
  • Upgrade operator-sdk to 0.15
  • Update build settings (docker, makefile, etc)

The commit messages in the new PR are much more explicit about what's going on.

@honza
Copy link
Copy Markdown
Member Author

honza commented Feb 27, 2020

Closing in favor of #444

@honza honza closed this Feb 27, 2020
dtantsur pushed a commit to dtantsur/baremetal-operator that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants