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

Investigate Go Modules or other solutions for refactoring Backend plugins #2815

Closed
irfanhabib opened this issue Aug 10, 2018 · 7 comments
Closed
Assignees

Comments

@irfanhabib
Copy link
Contributor

irfanhabib commented Aug 10, 2018

No description provided.

@KlapTrap
Copy link
Contributor

KlapTrap commented Aug 13, 2018

We use a non-standard go project structure due to how we implement our plugins. Can we use go modules to keep a standard project structure and implement our plugin mechanism?

As of writing this, Go Modules are an experimental feature of 1.11 (https://github.com/golang/go/wiki/Modules) with the plan for them to be introduced in 1.12.

@nwmac nwmac self-assigned this Aug 14, 2018
@nwmac
Copy link
Contributor

nwmac commented Aug 14, 2018

Referencing @aeijdenberg PR - #2662.

Go modules is coming and is one solution. Having investigated and prototyped this, it would allow us to switch from glide for dependencies and keep the structure we have with plugins. Unfortunately the tool support is not there yet - so, for example, autocomplete does not work in vscode - it seems that the gocode tool does not support modules (and has some issues since 1.10) - nsf/gocode#509.

Therefore, the proposal is to use dep for dependencies for now and revisit modules when the tooling catches up.

We will look at how best to allow developers to add their own customisations to the backend in a way that avoids merge conflicts when they rebase. The main issue being allowing customisations to add dependencies.

I think the more standard layout is more beneficial than the customisation approach we previously had in place that was not really used.

@aeijdenberg We plan to go along the path you described with a couple of variations:

  • Rather than have components and plugins in the root folder, we would:

    • rename src/backend to src/jetstream (jetstream being the new backend name - instead of portal-proxy)
    • Get rid of app-core and move the core code to src/jetstream
    • Creare src/jetstream/plugins
    • Move all plugins into src/jetstream/plugins
    • Update imports - they'll be along the lines of cloudfoundry-incubator/stratos/src/jetstream/....
  • We would not commit the vendor folder to repo - upon checkout, developers will need to run dep ensure

Feedback please - we will make these changes in the coming days if there is no push back.

@aeijdenberg I have created a variant of you script which migrates the repo to this new structure with the changes above - its here: https://raw.githubusercontent.com/cloudfoundry-incubator/stratos/backend-rework/build/tools/backend-rejig.sh

@aeijdenberg
Copy link
Contributor

@nwmac - looks good.

A few issues I found when running the script on Ubuntu:

  1. git rm src/jetstream/app-core should be rmdir src/jetstream/app-core (as it's an empty dir, and not stored in git, and gives an error when running)

  2. The sed -i '' seemed to need to be sed -i'' on Ubuntu

  3. The 3 import search and replaces can be simplified to 2:

# Fix logrus imports to match capitalization used by the CF CLI library (else dep will fail)
find . -name "*.go" | xargs sed -i'' 's/github.com\/Sirupsen\/logrus/github.com\/sirupsen\/logrus/g'

# Update imports
find . -name "*.go" | xargs sed -i'' 's/github.com\/SUSE\/stratos-ui/github.com\/cloudfoundry-incubator\/stratos\/src\/jetstream/g'

But at the end, the binary builds in a standard GOPATH, which is great!

I've seen projects go both ways with respect to committing the vendor dir... The nice thing about dep is that it prunes tests and unused packages by default, so the vendor dir tends to be much smaller than those produced by Glide in my experience.

@nwmac
Copy link
Contributor

nwmac commented Aug 15, 2018

@aeijdenberg Thank you. I'll make those updates. I had to pin a couple of dependencies to get the tests to all pass - a few libraries have moved on - we'll update those and update the test code after we've got the refactor in.

I'm working on getting build working for the various ways we can deploy.

@nwmac
Copy link
Contributor

nwmac commented Aug 20, 2018

@aeijdenberg I've got 2 PRs up for this now:

First one only has the build changes, not the source moves/renames, so its easier to see what has changed.

@aeijdenberg
Copy link
Contributor

Looks great @nwmac - thanks for getting this in! I'll go through and close / tidy up some of the related PRs we had.

@nwmac
Copy link
Contributor

nwmac commented Nov 13, 2018

Done

@nwmac nwmac closed this as completed Nov 13, 2018
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

No branches or pull requests

4 participants