Adding in customized user-agent #69
Conversation
|
Can one of the admins verify this patch? |
Makefile
Outdated
|
|
||
| all: cbuild container | ||
|
|
||
| cbuild: | ||
| $(DOCKER) run --rm -v $(ROOT_DIR):$(BUILDMNT) $(EXTRA_MNTS) -w $(BUILDMNT) -e SKIP_TESTS=$(SKIP_TESTS) $(BUILD_IMAGE) /bin/sh -c 'make local verify test' | ||
|
|
||
| container: cbuild | ||
| $(DOCKER) build -t $(REGISTRY)/$(PROJECT):latest -t $(REGISTRY)/$(PROJECT):$(VERSION) . | ||
| $(DOCKER) build -t $(REGISTRY)/$(PROJECT):latest -t $(REGISTRY)/$(PROJECT):$(VERSION) -t $(REGISTRY)/$(PROJECT):$(GIT_SHA) . |
There was a problem hiding this comment.
I'd prefer not to tag with the sha
Makefile
Outdated
|
|
||
| container-local: $(BINARIES) | ||
| $(DOCKER) build -t $(REGISTRY)/$(PROJECT):latest -t $(REGISTRY)/$(PROJECT):$(VERSION) . | ||
| $(DOCKER) build -t $(REGISTRY)/$(PROJECT):latest -t $(REGISTRY)/$(PROJECT):$(VERSION) -t $(REGISTRY)/$(PROJECT):$(GIT_SHA) . |
There was a problem hiding this comment.
I'd prefer not to tag with the sha
|
add to whitelist |
Signed-off-by: Justin Nauman <justin.r.nauman@gmail.com>
pkg/client/client.go
Outdated
| return clientConfig, nil | ||
| } | ||
|
|
||
| // buildUserAgent builds a User-Agent string from given args. | ||
| func buildUserAgent(command, version, os, arch, commit string) string { |
There was a problem hiding this comment.
Now that we have buildinfo.GitTreeState, could you have it so the commit portion of the user agent string appends -dirty if it's dirty?
|
@skriss ptal |
|
No test, even for the user agent string generation? |
pkg/cmd/server/server.go
Outdated
| var kubeconfig string | ||
|
|
||
| var command = &cobra.Command{ | ||
| Use: "server", | ||
| Short: "Run the ark server", | ||
| Long: "Run the ark server", | ||
| Run: func(c *cobra.Command, args []string) { | ||
| s, err := newServer(kubeconfig) | ||
| s, err := newServer(kubeconfig, baseName+"-server") |
There was a problem hiding this comment.
nit: you could just use c.Parent().Name() and c.Name() to construct this rather than passing in the baseName and hardcoding "server"
|
otherwise LGTM, thanks! |
|
Do you want a unit?
…On Mon, Sep 18, 2017 at 1:19 PM Steve Kriss ***@***.***> wrote:
otherwise LGTM, thanks!
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#69 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAABYsJClBw86humy2aguT8Zb69ERV3qks5sjqYegaJpZM4PPTlj>
.
|
|
yeah, sure, should be super-quick. |
Signed-off-by: Justin Nauman <justin.r.nauman@gmail.com>
|
I don't know of a good way of "unit testing" the whole agent string without a lot of mocking for things like K8 context, etc. so I added in tests for the suffix & the agent. I think that meets your expectations. I went back and forth on something trivial, but curious your thoughts: I waffled between having In the end I went with centralizing it all into the Thoughts on how you would have approached it? |
|
Updates LGTM. Personally I probably would've either done it the way you did, or just put the logic inside the buildUserAgent function. Obviously if there's another part of the code that could use this logic, the former would be better. |
Fixes #14
Adding in custom user-agent that includes more relevant data from Ark.