Skip to content

Set default name for resources#97

Merged
caugustus-sourcegraph merged 4 commits into
mainfrom
caug/add-name
Apr 7, 2022
Merged

Set default name for resources#97
caugustus-sourcegraph merged 4 commits into
mainfrom
caug/add-name

Conversation

@caugustus-sourcegraph
Copy link
Copy Markdown
Contributor

Alternate implementation of #94 - sets a default name for all services so that we can easily reference resources.

The second commit updates most resources to use this default name, instead of a hardcoded version. I'm on the fence about how valuable this is.

Not included: service names and persistentVolumeClaim names remain hardcoded, instead of referencing the variable. I'm unclear what impact changing a service name would have on Sourcegraph's service discovery (most likely we can work around it by setting environment variables, but it's easier to avoid creating this problem). For PVC's, it seems risky to change the name if it had previously been created.

Checklist

Test plan

No change to rendered output before/after using default values.

@caugustus-sourcegraph caugustus-sourcegraph requested a review from a team April 6, 2022 21:15
@michaellzc
Copy link
Copy Markdown
Member

service names and persistentVolumeClaim names remain hardcoded, instead of referencing the variable. I'm unclear what impact changing a service name would have on Sourcegraph's service discovery (most likely we can work around it by setting environment variables,

I can confirm changing service names will break service discovery. We have way too many magic default env var e.g. GITSERV_URL (or something like that), and it's not fun to haunt each one down.

👍 let's just don't touch it.

changing PVC name should be fine, but there's no good reason to do it, so let's leave it.

defaultTag: 3.38.0@sha256:246c3d82072511f376049762056a3c82fce5dbc4a00f29f10f64373b5fe0a9a9
# -- Docker image name for the `cadvisor` image
name: "cadvisor"
# -- Name used by resources. Does not affect service names or PVCs.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, because name is a string it gets automatically added to the docs and there's no way to hide this.

@caugustus-sourcegraph caugustus-sourcegraph enabled auto-merge (squash) April 7, 2022 13:24
@caugustus-sourcegraph caugustus-sourcegraph merged commit c4e82d3 into main Apr 7, 2022
@caugustus-sourcegraph caugustus-sourcegraph deleted the caug/add-name branch April 7, 2022 13:24
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.

2 participants