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

Modularise the repository #33

Open
wants to merge 2 commits into
base: mainline
Choose a base branch
from

Conversation

chrisnovakovic
Copy link

@chrisnovakovic chrisnovakovic commented Feb 11, 2022

Issue #, if available: #1

Description of changes:

In PR #11, @bwagner5 refactored the code base as a Go module. Unfortunately, there were half a dozen other unrelated changes in the same PR, which have perhaps contributed to it not being merged. This PR is limited solely to the modularisation of the code, without the other changes that you may or may not want to accept.

  • The first commit (4403e02) modularises the repository using the same directory structure as Restructure Repo w/ Go Modules and Add Darwin arm64 support #11. It targets the same version of Go as the one used by the Dockerfile (1.15). Support scripts, files and documentation have been updated to reflect the modularisation and removal of the vendor/ directory, even though this takes place in the following commit.
  • The second commit (a413d1b) removes the vendored third-party code in vendor/. This is a separate commit because of how noisy the deletion of the vendorised code is compared with the changes made in the first commit.

make checkstyle fails, but it's currently failing on the mainline branch anyway (see #17), and it at least fails in the same way after these changes (in fact the new changes make it fail slightly less with Go 1.17). With checkstyle turned into a no-op in the Makefile, I was able to build working binaries with make build-linux-amd64 and a working Debian package with make package-deb-amd64. make quick-test behaves as expected, and passes.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

* Move packages containing entrypoints into the `cmd` package.
* Move all other packages into the `pkg` package.
* Declare all third-party dependencies in `go.mod`.
* Update support scripts/files to handle new repository layout.
@chrisnovakovic chrisnovakovic changed the title Modularise Modularise the repository Feb 11, 2022
@@ -26,7 +26,7 @@ package:: create-package-folder package-rpm-amd64 package-rpm-386 package-rpm-ar
release:: clean checkstyle release-test pre-release build prepack package copy-package-dependencies

clean:: remove-prepacked-folder
rm -rf build/* bin/ pkg/ vendor/bin/ vendor/pkg/ .cover/
rm -rf build/* bin/ pkg/ .cover/
Copy link

Choose a reason for hiding this comment

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

Suggested change
rm -rf build/* bin/ pkg/ .cover/
rm -rf build/* bin/ .cover/

It looks like some files that shouldn't be cleaned up have been moved into pkg

@Yangtao-Hua
Copy link
Contributor

Hi @chrisnovakovic,
Thank you for choosing session-manager-plugin and contribute to it. we already noticed it and some changes like the import path and upgrade to go-mod, these changes are meaningful and super useful, they are great reference for blocked customers. As it is a big change, we did not immediately merge it because need legal verification and some internal test.
Thanks again for the contribution!

Thanks,
Yangtao

@andymac4182
Copy link

@Yangtao-Hua Any idea when it will be merged or an answer as this would help enhance a lot of internal tooling that uses this tool.
Eg.
https://github.com/gjbae1212/gossm
https://github.com/tedsmitt/ecsgo

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