-
Notifications
You must be signed in to change notification settings - Fork 178
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
INTMDB-232: Fix user agent version #505
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you so much @MihaiBojin
@@ -42,7 +42,7 @@ websitefmtcheck: | |||
lint: | |||
@echo "==> Checking source code against linters..." | |||
# https://github.com/golangci/golangci-lint/issues/337 fixing error | |||
golangci-lint run ./$(PKG_NAME) -v --deadline=30m | |||
bin/golangci-lint run ./$(PKG_NAME) -v --deadline=30m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive-by: golangci-lint is installed under bin/
- there's no point relying on the tool being available in the system path. cc @gssbzn in case I'm missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 16 includes the bin folder in the PATH
(export PATH := ./bin:$(PATH)
) for convenience, no need to revert this just curious if that no longer works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't work for me on Mac, but it worked in GH actions
|
||
digest "github.com/mongodb-forks/digest" | ||
"github.com/mongodb-forks/digest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive-by fix - unnecessary aliasing
@@ -1,4 +1,4 @@ | |||
package mongodbatlas | |||
package version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved under root / following the same directory structure as the AWS provider
@@ -14,7 +14,7 @@ builds: | |||
flags: | |||
- -trimpath | |||
ldflags: | |||
- -s -w -X mongodbatlas/version.ProviderVersion={{.Version}} | |||
- -s -w -X 'github.com/mongodb/terraform-provider-mongodbatlas/version.ProviderVersion={{.Version}}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProviderVersion was not being replaced, without the full repo path being specified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -38,7 +39,7 @@ func (c *Config) NewClient(ctx context.Context) (interface{}, diag.Diagnostics) | |||
|
|||
client.Transport = logging.NewTransport("MongoDB Atlas", transport) | |||
|
|||
optsAtlas := []matlasClient.ClientOpt{matlasClient.SetUserAgent("terraform-provider-mongodbatlas/" + ProviderVersion)} | |||
optsAtlas := []matlasClient.ClientOpt{matlasClient.SetUserAgent("terraform-provider-mongodbatlas/" + version.ProviderVersion)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[q] this is the user agent we do for the cli
var userAgent = fmt.Sprintf("%s/%s (%s;%s)", config.ToolName, version.Version, runtime.GOOS, runtime.GOARCH)
should we make something similar, ie: include os and arch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense. Will update later, but if you want to make the change directly on the PR it would be even better, since I won't be in front of a computer for the next few hours. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought @gssbzn, if we made this change, the UA would be reported as:
userAgent: terraform-provider-mongodbatlas/0.9.1-29-g13eb1636 (darwin;amd64) go-mongodbatlas/0.10.1 (darwin;amd64)
(since we add the OS/ARCH via the go client)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to merge the PR as is / I don't think duplicating that info makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally forgot we were doing that in the client, merge away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM questions and clarifications are not blocking
Thanks fro fixing this
Description
Please include a summary of the fix/feature/change, including any relevant motivation and context.
Link to any related issue(s):
Type of change:
Required Checklist:
Further comments
Confirmed UA is now reported correctly (Splunk):
Repro
main.tf
Reported UA