Slimmer /api package #5213

notnoop · 2019-01-18T21:06:17Z

Reduces api dependency footprint to ease client depedency management, specially in preparation to using go mod.

Following some internal conversation about nomad API, I noticed that the /api package surprisingly depends on many packages such as hashicorp's raft, lru, immutable-radix packages, and github.com/ugorji/go/codec library. github.com/ugorji/go/codec dependency causes pain to developers as it henders them from using a recent version, given that revision dates back to 2017-06-20.

With these changes, api package removes 30 dependencies, keeping dependency on 5 only. The difference in non-std dependencies from v0.8.7:

-github.com/armon/go-metrics
 github.com/gorhill/cronexpr
-github.com/hashicorp/consul/api
-github.com/hashicorp/errwrap
 github.com/hashicorp/go-cleanhttp
-github.com/hashicorp/go-immutable-radix
-github.com/hashicorp/go-msgpack/codec
-github.com/hashicorp/go-multierror
 github.com/hashicorp/go-rootcerts
-github.com/hashicorp/go-version
-github.com/hashicorp/golang-lru
-github.com/hashicorp/golang-lru/simplelru
-github.com/hashicorp/hcl
-github.com/hashicorp/hcl/hcl/ast
-github.com/hashicorp/hcl/hcl/parser
-github.com/hashicorp/hcl/hcl/scanner
-github.com/hashicorp/hcl/hcl/strconv
-github.com/hashicorp/hcl/hcl/token
-github.com/hashicorp/hcl/json/parser
-github.com/hashicorp/hcl/json/scanner
-github.com/hashicorp/hcl/json/token
-github.com/hashicorp/nomad/acl
 github.com/hashicorp/nomad/api/contexts
-github.com/hashicorp/nomad/helper
-github.com/hashicorp/nomad/helper/args
-github.com/hashicorp/nomad/helper/flatmap
-github.com/hashicorp/nomad/helper/uuid
-github.com/hashicorp/nomad/nomad/structs
-github.com/hashicorp/raft
-github.com/hashicorp/serf/coordinate
-github.com/mitchellh/copystructure
 github.com/mitchellh/go-homedir
-github.com/mitchellh/hashstructure
-github.com/mitchellh/reflectwalk
-github.com/ugorji/go/codec

fyi @blalor @jefferai

nomad/structs is an internal package and imports many libraries (e.g. raft, codec) that are not relevant to api clients, and may cause unnecessary dependency pain (e.g. `github.com/ugorji/go/codec` version is very old now). Here, we add a code generator that imports the relevant constants from `nomad/structs`. I considered using this approach for other structs, but didn't find a quick viable way to reduce duplication. `nomad/structs` use values as struct fields (e.g. `string`), while `api` uses value pointer (e.g. `*string`) instead. Also, sometimes, `api` structs contain deprecated fields or additional documentation, so simple copy-paste doesn't work. For these reasons, I opt to keep the status quo.

`api.MockJob` is a test utility, that's only used by `command/agent` package. This moves it to the package and removes it from the public API.

Embed pointer conversion functions in the API package to avoid unnecessary package dependency. `helper` package imports more dependencies relevant for internal use (e.g. `hcl`).

`helpers.FormatFloat` function is only used in `api`. Moving it and marking it as private. We can re-export it if we find value later.

hashicorp-cla · 2019-01-18T21:06:32Z

All committers have signed the CLA.

schmichael · 2019-01-18T21:57:52Z

api/nodes.go

@@ -224,7 +221,7 @@ func (n *Nodes) monitorDrainNode(ctx context.Context, nodeID string,
 			return
 		}

-		if node.Status == structs.NodeStatusDown {
+		if node.Status == NodeStatusDown {


Ah! I see what you meant the other day when you said linters could help. This should have never happened, but thanks to the magic of goimports and human muscle memory it happens very easily!

Thanks for catching it!

Oh, interesting! How would a linter have helped here?

In the past api/ should never have imported from nomad/structs, so something like go list -f {{ .Deps }} | grep nomad/structs finding a match could work as a linter.

I think the idea now is that api/ shouldn't import anything from anywhere else in Nomad at all, so a similar linter that inspects api/'s deps on commit would be helpful.

Nomad few packages (e.g. plugins, api) that represent public "stable" API, and we have some opinions about the graph of package dependencies and layout. I proposed in a meeting formalizing it and adding a linter to enforce them.

nomad/structs/structs.go

Given that the values will rarely change, specially considering that any changes would be backward incompatible change. As such, it's simpler to keep syncing manually in the rare occasion and avoid the syncing code overhead.

preetapan · 2019-01-19T02:09:03Z

api/tasks_test.go

@@ -6,7 +6,6 @@ import (
 	"testing"
 	"time"

-	"github.com/hashicorp/nomad/helper"
 	"github.com/hashicorp/nomad/nomad/structs"


Having tests depend on the structs package and use the same constants (e.g DefaultBatchReschedulePolicy) but having the api package copy the values over so that they have to be manually synced seems wrong to me. What were we trying to optimize for here?

I opted to have the tests check that api package have the same values as structs version, so tests fail if they are out of sync.

Given that vendoring tools (e.g. govendor) prune out test files, by default, the dependency on structs package wouldn't leak into clients of this package.

Does this make sense?

github-actions · 2023-02-19T02:18:05Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Mahmood Ali added 4 commits January 18, 2019 14:51

api: remove MockJob from exported functions

a5d60fd

`api.MockJob` is a test utility, that's only used by `command/agent` package. This moves it to the package and removes it from the public API.

api: embed pointer conversion functions

41c3250

Embed pointer conversion functions in the API package to avoid unnecessary package dependency. `helper` package imports more dependencies relevant for internal use (e.g. `hcl`).

api: move formatFloat function

b1293a8

`helpers.FormatFloat` function is only used in `api`. Moving it and marking it as private. We can re-export it if we find value later.

schmichael reviewed Jan 18, 2019

View reviewed changes

schmichael approved these changes Jan 18, 2019

View reviewed changes

nomad/structs/structs.go Outdated Show resolved Hide resolved

notnoop force-pushed the b-api-separate branch from 935984e to ce52547 Compare January 18, 2019 23:41

api: avoid codegen for syncing

5e18538

Given that the values will rarely change, specially considering that any changes would be backward incompatible change. As such, it's simpler to keep syncing manually in the rare occasion and avoid the syncing code overhead.

notnoop force-pushed the b-api-separate branch from ce52547 to 5e18538 Compare January 18, 2019 23:52

notnoop merged commit f7e7b99 into master Jan 19, 2019

notnoop deleted the b-api-separate branch January 19, 2019 02:04

preetapan reviewed Jan 19, 2019

View reviewed changes

notnoop mentioned this pull request Apr 28, 2019

divest /api from rest of nomad internal packages #5622

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slimmer /api package #5213

Slimmer /api package #5213

notnoop commented Jan 18, 2019

hashicorp-cla commented Jan 18, 2019 •

edited

Loading

schmichael Jan 18, 2019

blalor Jan 18, 2019

schmichael Jan 18, 2019

notnoop Jan 18, 2019

preetapan Jan 19, 2019

notnoop Jan 19, 2019 •

edited

Loading

github-actions bot commented Feb 19, 2023

Slimmer /api package #5213

Slimmer /api package #5213

Conversation

notnoop commented Jan 18, 2019

hashicorp-cla commented Jan 18, 2019 • edited Loading

schmichael Jan 18, 2019

Choose a reason for hiding this comment

blalor Jan 18, 2019

Choose a reason for hiding this comment

schmichael Jan 18, 2019

Choose a reason for hiding this comment

notnoop Jan 18, 2019

Choose a reason for hiding this comment

preetapan Jan 19, 2019

Choose a reason for hiding this comment

notnoop Jan 19, 2019 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 19, 2023

hashicorp-cla commented Jan 18, 2019 •

edited

Loading

notnoop Jan 19, 2019 •

edited

Loading