Move the watch package into the api module #5664
Conversation
|
The elephant in the room is that this will break all pre-go-modules dependent builds. The counter argument is that per #5604 it is already broken. While this has prospect of fixing it longer term and doesn't really make it more broken. Is there any way we can make this cleaner so that the existing import path doesn't just stop existing (e.g. symlink) that actually makes anything better? And what are the pros and cons vs fixing up the vendor dir to not include the api package? That seems relatively easy too. It seems to only do so because I think overall this may well be the best solution - it's a better fit there. But would be good to see the alternatives discussed here so we can remember why we choose this path later! It feels like every move we've made in go mod realm has resulted in consequences we didn't foresee which makes me wonder what effects this might have that we've not figured out yet. For example did you test that this imports from a separate project and builds correctly with and without go modules after the PR is applied? What does a dependent project that relies on the old path see? (presumably just a 404 or similar)? |
|
Also, cf. #5636 - an argument for the move. |
|
@banks This definitely does break compatibility but in a way that its just a few imports that need to be rewritten. Users could always use my One other idea I considered that probably wouldn't break anything would be to have the watch package be its own submodule. This should fix the existing problems. When discussing this yesterday there was some push back against creating more nested modules as a way to fix our nested modules issue. We also could go down the route of creating separate repos for the api, watch and sdk submodules and then simply pushing to those repos nightly or during releases. Anyone currently using the watch package wouldn't have to change but they could start using something like github.com/hashicorp/consul-watch or something like that to get their transitive dependencies greatly reduced. Lastly, I wouldn't rely on symlinks. Especially because not all filesystems support symlinks (NTFS on Windows). Additionally the various go tooling do not usually handle symlinks (which is why I had to bind mount things into the GOPATH for building pre-modules instead of just symlinking). |
It was already just a thin wrapper around the API anyways. The biggest change was to the testing. Instead of using a test agent directly from the agent package it now uses the binary on the PATH just like the other API tests. In order for this to happen with minimal duplicated code the API package now has a few exported MakeTestClient* functions to handle spawning a test agent to run against. The other big changes were to fix up the connect based watch tests so that we didn’t need to pull in the connect package (and therefore all of Consul)
go vet was complaining about import cycles
mkeeler
force-pushed
the
modules/move-watch
branch
from
624b459 to
6945b0c
Compare
|
Verified that a non-module go 1.11.5 build using the watch package in the updated location now builds properly. |
|
The repro in #5713 was very helpful. Without getting rid of the vendoring of our own code no amount of moving things around will let it compile. I also tested the same example outside of the GOPATH with go 1.12.1 and it built/ran as expected. |
It was already just a thin wrapper around the API anyways. The biggest change was to the testing. Instead of using a test agent directly from the agent package it now uses the binary on the PATH just like the other API tests. In order for this to happen with minimal duplicated code the API package now has a few exported MakeTestClient* functions to handle spawning a test agent to run against.
The other big changes were to fix up the connect based watch tests so that we didn’t need to pull in the connect package (and therefore all of Consul)
Either this or an alternative solution will need to be put in before the next release.
This also fixes the makefile so that we run all the tests not just ones in the main module (
go list ./...doesn't seem to traverse into nested submodules)The dependencies of the API package remain unchanged and the code included for the watching is minimal. The oddest part is that the API package needed to export some additional functions to allow setting up the agent for the watch tests to execute against. I tried exporting them in api_test.go but then the api/watch package couldn’t see them. So the two options were to move them out of a *_test.go file or get rid of the subpackage all together and move the watch code into the api package. If there are other ways to do that please let me know.
Fixes #5604
Fixes #5713