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

Convert components to a go module #933

Open
winkingturtle-vmw opened this issue May 30, 2024 · 6 comments
Open

Convert components to a go module #933

winkingturtle-vmw opened this issue May 30, 2024 · 6 comments

Comments

@winkingturtle-vmw
Copy link
Contributor

winkingturtle-vmw commented May 30, 2024

Proposed Change

As a developer
I would like the ability to build diego-release's components (e.g. bbs) as a standalone module
So that when I run go install code.cloudfoundry.org/bbs/cmd/bbs@latest and it will install the latest version of the binary

diego-release components were built at a time before go modules existence. It was built with the assumption of GOPATH. As a result, most modules import each other, creating a circular dependencies where bbs needs to import locket and lcoket needs to import bbs. This is because it was easier to use helper libraries that each repo created originally. In order to be able to do this work we need to do the following to break out the circular import and allow each of those modules to be independent.

Here are the list of components that will have to be converted

auction
auctioneer
bbs
buildpackapplifecycle
cacheddownloader
cfdot
diego-ssh
dockerapplifecycle
ecrhelper
executor
fileserver
healthcheck
inigo
localdriver
locket
operationq
rep
route-emitter
volman
vizzini
workpool
  • For each of the repo above, we need to figure where it's being imported (e.g. bbs). Some of these imports are happening silently in releases that are not showing up in pkg.go.dev.
  • We then need to figure out why these internal repos are importing diego's repositories. In the case of bbs, most of the consumers are just looking for models or sqlhelpers. In this case, we would extract out the logic into it's own independent repository and update downstream consumers.
  • For a given repo, we then need to break out circular imports. In the case of bbs, it's importing locket, auctioneer, ecrhelper, executor, inigo, and workpool. Here is a branch for bbs and a branch for locket that removed the circular dependency between the two of them.
  • After we finish migrating each component, we then need to change the diego-release to import these modules via it's main go-module instead of submodules.

Acceptance criteria

diego-release will no longer have any submodules and I can deploy diego-release with cf-deployment, so that I cf push and run all acceptance tests.

Related links

@maxmoehl
Copy link
Member

Can you share a bit more detail about why the cyclic dependencies break what you are trying to achieve? If they are on the package level I would expect that go refuses to build them no matter how the modules are set up. But if they are cyclic on the module level they should not cause issues if I understand the situation correctly.

@winkingturtle-vmw
Copy link
Contributor Author

@maxmoehl hmm, I am not sure if I follow your question, but let me re-iterate it differently. I am only going to use the following two code.cloudfoundry.org/bbs and code.cloudfoundry.org/locket modules only. These modules are used outside of diego-release, so it can't be internal and will need to have publicly addressable URL. Currently when there is no go-moded version/release of these repos, we are falling on a GOPATH backward compatible and unadvertised path to get the latest version of each. When they are both converted to a module, they import each other (BBS imports Locket and vice versa) and the cycle causes an issue.

That said, you are more than welcome to experiment with other ways of solving this problem. I am only sharing what has been our experience solving this issue thus far.

@maxmoehl
Copy link
Member

maxmoehl commented Jul 3, 2024

When they are both converted to a module, they import each other (BBS imports Locket and vice versa) and the cycle causes an issue.

I would like to know which issue this causes, do you have some error message you encountered?

@winkingturtle-vmw
Copy link
Contributor Author

When we first looked at this it was 3 years ago when we had to migrate to a go.mod and migrate off of GOPATH, so I don't have the specific error messages I was seeing. If we now have the community support behind this, we should re-start that effort and document it along the way for future.

@maxmoehl
Copy link
Member

maxmoehl commented Jul 4, 2024

Okay, I'll try to reserve some time to play around and report back with my findings.

@maxmoehl
Copy link
Member

After some investigations into this issue I think the current setup is good enough and splitting the module definitions would probably lead to additional effort with limited return:

  • CF internal dependencies which are currently just submodules would need to become proper dependencies that are versioned and released independently.
  • We would have to bump a lot more modules instead of just one in the release.
  • The initial split would require some refactoring which could be non-trivial.
  • The repositories would still not be stand-alone as most of the supporting scripts for tests etc. still live in the release.

I won't continue to investigate this topic for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

2 participants