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

feat(influx): extend CLI with versoin and user-agent #18344

Merged
merged 2 commits into from
Jun 2, 2020

Conversation

jsteenb2
Copy link
Contributor

@jsteenb2 jsteenb2 commented Jun 2, 2020

Closes: #18336
Closes: #14821


Acceptance Criteria

  • all http calls from influx CLI have a User-Agent header
  • User-Agent header should look like User-Agent: influx/$VERSION (<system-information>) Sha/$SHA Date/$BUILD_DATE

@jsteenb2 jsteenb2 requested review from desa and hoorayimhelping June 2, 2020 22:45
@jsteenb2 jsteenb2 requested a review from russorat June 2, 2020 22:47
@jsteenb2 jsteenb2 force-pushed the 18336/influx_useragent_header branch from f44e345 to 3aa8f1c Compare June 2, 2020 22:47
@jsteenb2 jsteenb2 merged commit fd28988 into master Jun 2, 2020
@jsteenb2 jsteenb2 deleted the 18336/influx_useragent_header branch June 2, 2020 23:09
var (
version = "dev"
commit = "none"
date = time.Now().UTC().Format(time.RFC3339)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fly-by comment: this isn't the build date (as noted in the PR description); it's the time that the influx process was started.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right. I borrowed this from the influxd binary here:

https://github.com/influxdata/influxdb/blob/master/cmd/influxd/main.go#L27

just noticed I dont' have the fmt.Sprintf, but that shouldn't matter yah?

It looks like the date isn't provided as an LD flag either, so that definitely can change 🤔. What do you think is the best way forward here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed the fmt.Sprint isn't necessary; time.Format already returns a string.

I think setting the date at link time is the correct approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we do set the date as a linker flag in goreleaser after all:

ldflags: -s -w -X main.version={{.Version}} -X main.commit={{.ShortCommit}} -X main.date={{.Date}}

ldflags: -s -w -X main.version={{.Version}} -X main.commit={{.ShortCommit}} -X main.date={{.Date}}

Any tooling we have to inspect the resulting date would know that version == "dev" indicates the date would be meaningless.

Nothing to change here after all.

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

Successfully merging this pull request may close these issues.

Add user-agent to influx CLI HTTP Calls cannot get version of influx CLI
3 participants