Skip to content

Conversation

@ryanslade
Copy link
Contributor

This removes the documentation of the endpoint flag but still allows it
to be used to override either the config or ennvironment variables.

This removes the documentation of the endpoint flag but still allows it
to be used to override either the config or ennvironment variables.
@efritz
Copy link
Contributor

efritz commented Jun 30, 2020

What is the original purpose of removing this (especially if we're hardening it with tests in this PR)?

@uwedeportivo
Copy link
Contributor

i'm using it here sourcegraph/deploy-sourcegraph#687

Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

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

Approve the code, but addressing @efritz question in the PR description and links to tickets etc. would be great to have :)

Comment on lines +155 to +165
oldToken := os.Getenv("SRC_ACCESS_TOKEN")
defer func() { os.Setenv("SRC_ACCESS_TOKEN", oldToken) }()
oldEndpoint := os.Getenv("SRC_ENDPOINT")
defer func() { os.Setenv("SRC_ENDPOINT", oldEndpoint) }()

if err := os.Setenv("SRC_ACCESS_TOKEN", test.envToken); err != nil {
t.Fatal(err)
}
if err := os.Setenv("SRC_ENDPOINT", test.envEndpoint); err != nil {
t.Fatal(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a bit easier to parse visually and read 😄 But, I was in the mood to write a tiny function just now, feel free to ignore:

Suggested change
oldToken := os.Getenv("SRC_ACCESS_TOKEN")
defer func() { os.Setenv("SRC_ACCESS_TOKEN", oldToken) }()
oldEndpoint := os.Getenv("SRC_ENDPOINT")
defer func() { os.Setenv("SRC_ENDPOINT", oldEndpoint) }()
if err := os.Setenv("SRC_ACCESS_TOKEN", test.envToken); err != nil {
t.Fatal(err)
}
if err := os.Setenv("SRC_ENDPOINT", test.envEndpoint); err != nil {
t.Fatal(err)
}
setEnvVar := func(name, val string) {
old := os.Getenv(name)
if err := os.Setenv(name, val); err != nil {
t.Fatal(err)
}
t.Cleanup(func() { os.Setenv(name, old) })
}
setEnvVar("SRC_ACCESS_TOKEN", test.envToken)
setEnvVar("SRC_ENDPOINT", test.envEndpoint)

if err != nil {
t.Fatal(err)
}
return filePath, func() { os.RemoveAll(tmpDir) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you already have t at hand, you could do this:

Suggested change
return filePath, func() { os.RemoveAll(tmpDir) }
t.Cleanup(func() { os.RemoveAll(tmpDir) })
return filePath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used t.Cleanup originally but src-cli is using an older version of Go in the build pipeline.

Let me check how easy it is to update that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comments and commits, I couldn't get Appveyor to use Go 1.14

Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
@ryanslade
Copy link
Contributor Author

@efritz The original issue was this one: #222

We're also deprecating the config file: #156

After looking at both of these it seemed to make sense to move towards only using environment variables.

@ryanslade
Copy link
Contributor Author

Nothing I've tried seems to get Appveyor to use Go 1.14 so I'm giving up for now and I'll create an issue

@ryanslade ryanslade merged commit fd5f1ac into master Jul 1, 2020
scjohns pushed a commit that referenced this pull request Apr 24, 2023
* Deprecate -endpoint flag

This removes the documentation of the endpoint flag but still allows it
to be used to override either the config or ennvironment variables.

* Update changelog.

* Update cmd/src/main.go

Co-authored-by: Thorsten Ball <mrnugget@gmail.com>

* Clean up CHANGELOG

* Use t.Cleanup

* Use Go 1.14

* Use Go 1.14 in Travis too

* Try specific version of Go for Appveyor

* Try example appveyor.yml

* Revert "Try example appveyor.yml"

This reverts commit 5f86ece.

* One more attempt, use different GOPATH

Apparently Go 1.14.3 is installed by default:
https://www.appveyor.com/docs/windows-images-software/#golang

* Revert back to being Go 1.13 compatible

Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
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.

5 participants