Repository navigation
deploy/substrate: drop the legacy SCION_HUB_ENDPOINT env from the broker - #3430
Conversation
The substrate broker runs broker-only and already takes its hub from server.broker.hub_endpoint in its ConfigMap, with the same value. The SCION_HUB_ENDPOINT env entry was a lower-precedence fallback that the broker's hub resolution never reached, and the server's legacy hub endpoint fallback is being retired. Add tests that render the real manifest, load its settings the way server start does in broker-only hosted mode, and check that the broker's hub endpoint is the ConfigMap value whether the env var is unset, set to the same value, or set to a different one, and that the container hub settings do not depend on it.
…e env var SCION_HUB_ENDPOINT also marks the process as a hub context. From a working directory outside any project, the default project is then a synthetic <cwd>/.scion that does not exist; without it, the default is the global directory, which is already loaded. Run the substrate broker resolution test from such a directory and assert the default project in each case.
Start each case from empty SCION_HUB_ENDPOINT, SCION_HUB_URL and SCION_PROJECT_ID, so only the case's own value can make the process a hub context, independent of the package's TestMain.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the legacy SCION_HUB_ENDPOINT environment variable fallback from the substrate broker deployment manifest and adds a comprehensive test suite to verify that the broker's hub endpoint resolution remains correct. The review feedback suggests a minor improvement in the new test file to check the error returned by loadServerSettings instead of ignoring it, which will prevent potential nil-pointer dereferences.
|
|
||
| cfg, err := config.LoadGlobalConfig("") | ||
| require.NoError(t, err) | ||
| settings, _ := loadServerSettings("") |
There was a problem hiding this comment.
The error returned by loadServerSettings is ignored. Checking this error ensures that any configuration loading issues during the test are caught immediately and failed with a clear message, rather than potentially causing nil-pointer dereferences or unexpected behavior later in the test.
settings, err := loadServerSettings("")
require.NoError(t, err)… test loadServerSettings only logs a load error and falls back to empty settings, so a broken fixture would go unnoticed. Call the loader it wraps, config.LoadSettings, and require no error.
1 participant
Refs ptone#4629
Moves the substrate broker manifest off the legacy hub endpoint variable, ahead of removing the server's fallback to it.
Tests: a table test of the hub endpoint resolvers with the substrate inputs, and go build ./cmd/... pass.