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

rename init packages and onboard init to go modules #3146

Merged
merged 1 commit into from
Mar 11, 2022

Conversation

lydiafilipe
Copy link
Contributor

@lydiafilipe lydiafilipe commented Mar 10, 2022

Summary

This onboards ecs-init to go modules. There are quite a few changes here, to summarize them

  • replace ecs init path references in scripts/init package
  • created the go module and go sum files
  • vendor directory updated
  • dep-related files and make target removed
  • tweaked the gobuild script to remove setting gopath

Implementation details

Replaced the ecs init package paths by running grep -rl github.com/aws/amazon-ecs-init . | xargs sed -i 's/github.com\/aws\/amazon-ecs-init/github.com\/aws\/amazon-ecs-agent/g'

Onboarded to go modules with
go mod init
go mod tidy

Had to add a replacement: replace github.com/coreos/go-systemd => github.com/coreos/go-systemd/v22 v22.0.0. This is due to a dependency on github.com/docker/go-plugins-helpers, which has not onboarded to go modules or semantic versioning for some of its imports.

After that, ran go mod tidy again. Had to replace some packages for which the versioned changed compared to what was in our dep files.

Then deleted the vendor directory and ran go mod vendor.

Additionally, had to remove setting GOPATH in our init build script (since that cases conflicts when using go modules). Also removed dep stuff in Makefile.

Testing

  • made/tested the change in the init package itself
  • successfully ran make rpm-in-docker and make generic-rpm in that package, and used the former rpm in an ami build (against which I ran functional tests)
  • built changes in Koji and, similarly to above, used the result rpms in ami builds and ran functional tests against those

I replicated those changes in this repository.

Also verified the github workflow for the same changes in the init repository succeeds

New tests cover the changes: no

Description for the changelog

rename init packages and onboard init to go modules

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

.travis.yml Outdated Show resolved Hide resolved
Copy link
Member

@fierlion fierlion left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@chienhanlin chienhanlin left a comment

Choose a reason for hiding this comment

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

Q. See some golang.org files are empty, like ecs-init/vendor/golang.org/x/sys/unix/mksyscall.pl, are they supposed to be empty? Thanks!

@lydiafilipe
Copy link
Contributor Author

lydiafilipe commented Mar 10, 2022

Q. See some golang.org files are empty, like ecs-init/vendor/golang.org/x/sys/unix/mksyscall.pl, are they supposed to be empty? Thanks!

Interesting that they are empty in the diff, they are not actually empty locally or in my branch - (eg https://github.com/lydiafilipe/amazon-ecs-agent/blob/init-gomodules/ecs-init/vendor/golang.org/x/sys/unix/mksyscall.pl). Regardless, these were all generated automatically by go mod vendor

@lydiafilipe lydiafilipe merged commit 7163b62 into aws:initAgentIntegration Mar 11, 2022
fierlion pushed a commit to fierlion/amazon-ecs-agent that referenced this pull request May 31, 2022
fierlion pushed a commit to fierlion/amazon-ecs-agent that referenced this pull request May 31, 2022
rsheik29 pushed a commit to rsheik29/amazon-ecs-agent that referenced this pull request Jul 11, 2022
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