Divest api/ package of deps elsewhere in the nomad repo. #5488
Conversation
This will allow making api/ a module without then pulling in the external repo, leading to a package name conflict. This required some migration of tests to an apitests/ folder (can be moved anywhere as it has no deps on it). It also required some duplication of code, notably some test helpers from api/ -> apitests/ and part (but not all) of testutil/ -> api/testutil/. Once there's more separation and an e.g. sdk/ folder those can be removed in favor of a dep on the sdk/ folder, provided the sdk/ folder doesn't depend on api/ or /.
There was a problem hiding this comment.
Approach lgtm, and testutil duplication seems fine and we have that pattern elsewhere too for API package (for structs), and we can clean up with the sdk later on without impacting users.
Also, should api/testutil packages be moved to internal namespace (e.g. api/internal/testutil? Avoids leaking tests in public API and allows us to move packages around without affecting users.
lgtm, but would be nice to have a second review too (@schmichael ?), for awareness and just I missed anything.
| @@ -7,7 +7,7 @@ import ( | |||
|
|
|||
| "time" | |||
|
|
|||
| "github.com/hashicorp/nomad/helper/uuid" | |||
There was a problem hiding this comment.
In a follow-up PR, I'll nuke helper/uuid for hashicorp/go-uuid? Given that we already depend on go-uuid (as showed by vendor file), it doesn't make sense to have our own helper package.
| @@ -46,43 +44,6 @@ func TestJobs_Register(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestJobs_Parse(t *testing.T) { | |||
There was a problem hiding this comment.
to call it out for future reviews, this and many removed tests were moved to apitests/
There was a problem hiding this comment.
Looks good. Confirmed only test files are changed, the uuid modification is minor:
$ git diff --name-only 7feee07d8f353f4bc83a19d25bb6906ed98edb0c | grep -v -e _test.go -e api/internal
vendor/github.com/hashicorp/go-uuid/README.md
vendor/github.com/hashicorp/go-uuid/go.mod
vendor/github.com/hashicorp/go-uuid/uuid.go
vendor/vendor.json
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This will allow making api/ a module without then pulling in the
external repo, leading to a package name conflict.
This required some migration of tests to an apitests/ folder (can be
moved anywhere as it has no deps on it). It also required some
duplication of code, notably some test helpers from api/ -> apitests/
and part (but not all) of testutil/ -> api/testutil/.
Once there's more separation and an e.g. sdk/ folder those can be
removed in favor of a dep on the sdk/ folder, provided the sdk/ folder
doesn't depend on api/ or /.