refactor: validate and parse the endpoint and proxy at program load

[peterguy]

Summary

Parse the endpoint URL at program load instead of passing around an unparsed string and needing to check/parse it in multiple places.

Parse the proxy url at program load, supporting HTTP_PROXY, HTTPS_PROXY and NO_PROXY in addition to SRC_PROXY. SRC_PROXY takes precedence.

Changes

config struct (cmd/src/main.go)

• Split out a configFromFile struct containing the JSON elements
• Un-export fields (all consumers are in package main)
• Use endpointURL - Endpoint and Proxy are now in configFromFile
• Parse endpointURL in readConfig - consumers use the parsed URL

api.ClientOpts (internal/api/api.go)

• Endpoint string → EndpointURL *url.URL
• Use JoinPath for URL construction instead of string concatenation with TrimRight

login command (cmd/src/login.go)

• loginCmd now uses the config's endpointURL
• Endpoint conflict detection moved from loginCmd into the handler, where the CLI arg is parsed and compared before entering the command

code_intel_upload (cmd/src/code_intel_upload.go)

• makeCodeIntelUploadURL uses a stack copy (u := *cfg.endpointURL) instead of re-parsing the URL
• Error return removed since there's nothing that can fail

Tests

• Updated all test call sites to pass *url.URL

Testing

All affected packages pass:

ok  github.com/sourcegraph/src-cli/cmd/src
ok  github.com/sourcegraph/src-cli/internal/api
ok  github.com/sourcegraph/src-cli/internal/batches/repozip
ok  github.com/sourcegraph/src-cli/internal/batches/executor

[peterguy]

The existence of this check calls into question the command-line parameter - SRC_ENDPOINT (or the value from the config file) is the source[1] of truth, forcing the user to put the same value on the command line.
Adding to the confusion is that if there is nothing in the config file or in SRC_ENDPOINT, cfg.endpointURL defaults to https://sourcegraph.com, which is probably wrong pretty much all of the time, and still invalidates anything different on the command line.
I kept the error condition and message (although I did change the semantics by changing from an exit error to a usage error) because I didn't want to rip that rug out from under anyone at this point.
Thoughts?

[1] no pun intended, but I do like it

[bahrmichael]

I think it's reasonable to keep backwards maintainability in this PR, but follow up with one to make the breaking changes. That makes it easier to isolate and revert if necessary.

[keegancsmith]

I believe you are changing the behaviour here. The old behaviour ignored SRC_ENDPOINT for login if the argument was set. The old error message was misleading

Edit: I see in the loginCmd function they checked if cfg.Endpoint is the same. I imagine that was a mistake. I agree with you about how this should work. But for now lets keep the same behaviour.

[peterguy]

For illustration, here's how it works currently (before these changes):

▶ SRC_ENDPOINT=https://sourcegraph.coursegraph.com src login https://sourcegraph.com

❌ Problem: The configured endpoint is https://sourcegraph.coursegraph.com, not https://sourcegraph.com.

🛠  To fix: Create an access token by going to https://sourcegraph.com/user/settings/tokens, then set the following environment variables in your terminal:

   export SRC_ENDPOINT=https://sourcegraph.com
   export SRC_ACCESS_TOKEN=(your access token)

   To verify that it's working, run the login command again.

And here's how it works after these changes:

▶ SRC_ENDPOINT=https://sourcegraph.coursegraph.com ./src login https://sourcegraph.com
error: The configured endpoint is https://sourcegraph.coursegraph.com, not https://sourcegraph.com

'src login' helps you authenticate 'src' to access a Sourcegraph instance with your user credentials.

Usage:

    src login SOURCEGRAPH_URL

Examples:
...

[peterguy]

The config file has been deprecated for a long time, but if it ever is finally removed, the ability to set additional headers will go also. Do we need to keep the ability to set additional headers around? If so, we should probably introduce yet another environment variable for those and delay the config file removal. If not, let's complete the config file deprecation!

[bahrmichael]

See https://github.com/sourcegraph/src-cli/pull/1267/changes#r2903847768

[peterguy]

This change is part of the following stack:

• refactor: validate and parse the endpoint and proxy at program load #1267 ◀
  • Add support for HTTP_PROXY, HTTPS_PROXY, and NO_PROXY #1260

_{Change managed by git-spice.}

[bahrmichael]

I think there's a bug in https://github.com/sourcegraph/src-cli/pull/1267/changes#r2903861064. But apart from that good to go, so I'll approve to unblock.

[keegancsmith]

switch to using JoinPath?

[peterguy]

Maybe? It is a pretty complex concatenation. Here are a few options for comparison:
Current:

		executionURL := fmt.Sprintf(
			"%s/%s/batch-changes/%s/executions/%s",
			cfg.endpointURL,
			strings.TrimPrefix(namespace.URL, "/"),
			batchChangeName,
			batchSpecID,
		)

Proposal A:

		executionURL := cfg.endpointURL.JoinPath(
			fmt.Sprintf(
				"/%s/batch-changes/%s/executions/%s", namespace.URL,
				batchChangeName,
				batchSpecID,
			),
		).String()

Proposal B:

		executionURL := endpointURL.
			JoinPath(namespace.URL).
			JoinPath("batch-changes").
			JoinPath(batchChangeName).
			JoinPath("executions").
			JoinPath(batchSpecID).
			String()

I think proposal B is the easiest to read, but also it's nearly the same as the original fmt.Sprintf.

[keegancsmith]

approving, but agree with Michael's catch on the breaking change.