Skip to content
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

Load cluster domain as a config option. Fall back to default if not set #339

Merged
merged 11 commits into from
Aug 23, 2024

Conversation

fr6nco
Copy link
Contributor

@fr6nco fr6nco commented Feb 10, 2024

…vc.cluster.local if not set

What

Cluster domain is hardcoded. Load the cluster domain from an environment variable. If the environment variable is not set, fall back to the default svc.cluster.local

Currently very simple, just please take a look if this is something you would accept.
Test were not written, I could not find anything covering the cluster domain, so it should be added.
Helm chart to be updated, where we could set the cluster domain to a configmap or simply passing it as an extraEnvVar: {}

How

Cluster domain is read from env variable.

Breaking Changes

no

@fr6nco fr6nco requested a review from a team as a code owner February 10, 2024 22:17
@github-actions github-actions bot added the area/controller Issues dealing with the controller label Feb 10, 2024
@fr6nco fr6nco changed the title Load cluster domain from environment variable. Fall back to default s… Load cluster domain from an environment variable. Fall back to default if not set Feb 10, 2024
@salilsub
Copy link
Contributor

Hi @fr6nco . For cases like this we'd want to use a Helm chart override instead of using an environment variable. If you modified this PR/submitted a new PR using this approach, we'd accept that.

@github-actions github-actions bot added area/helm-chart Issues dealing with the helm chart area/release Issues relating to releases labels Feb 15, 2024
Copy link

👋 Looks like there are changes in the Helm Chart's Chart.yaml file. Upon merge, a new release of the helm chart will be created if the Chart's version was changed.

Copy link

👋 Looks like there are changes in the VERSION file. Upon merge, a new release of the docker image will be created. Please make sure the version is updated appropriately.

@fr6nco fr6nco changed the title Load cluster domain from an environment variable. Fall back to default if not set Load cluster domain as a config option. Fall back to default if not set Feb 15, 2024
Copy link

👋 Looks like there are changes in the Helm Chart's Chart.yaml file. Upon merge, a new release of the helm chart will be created if the Chart's version was changed.

Copy link

👋 Looks like there are changes in the VERSION file. Upon merge, a new release of the docker image will be created. Please make sure the version is updated appropriately.

Copy link

👋 Looks like there are changes in the Helm Chart's Chart.yaml file. Upon merge, a new release of the helm chart will be created if the Chart's version was changed.

Copy link

👋 Looks like there are changes in the VERSION file. Upon merge, a new release of the docker image will be created. Please make sure the version is updated appropriately.

Copy link

👋 Looks like there are changes in the Helm Chart's Chart.yaml file. Upon merge, a new release of the helm chart will be created if the Chart's version was changed.

Copy link

👋 Looks like there are changes in the VERSION file. Upon merge, a new release of the docker image will be created. Please make sure the version is updated appropriately.

@fr6nco
Copy link
Contributor Author

fr6nco commented Feb 15, 2024

I bit more commints than expected. It was reworked, where the option is passed down as a config option. No longer via an env variable. The option is exposed via the helm chart.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Mar 5, 2024

👋 Looks like there are changes in the Helm Chart's Chart.yaml file. Upon merge, a new release of the helm chart will be created if the Chart's version was changed.

Copy link

github-actions bot commented Mar 5, 2024

👋 Looks like there are changes in the VERSION file. Upon merge, a new release of the docker image will be created. Please make sure the version is updated appropriately.

Copy link

github-actions bot commented Mar 7, 2024

👋 Looks like there are changes in the Helm Chart's Chart.yaml file. Upon merge, a new release of the helm chart will be created if the Chart's version was changed.

Copy link

github-actions bot commented Mar 7, 2024

👋 Looks like there are changes in the VERSION file. Upon merge, a new release of the docker image will be created. Please make sure the version is updated appropriately.

Copy link
Contributor Author

@fr6nco fr6nco left a comment

Choose a reason for hiding this comment

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

Changelog changes removed as requested.

@github-actions github-actions bot removed the area/release Issues relating to releases label Aug 23, 2024
@jonstacks jonstacks requested a review from CK-Ward August 23, 2024 19:56
@jonstacks jonstacks self-assigned this Aug 23, 2024
@jonstacks jonstacks merged commit b391e08 into ngrok:main Aug 23, 2024
8 checks passed
@jonstacks jonstacks added this to the controller 0.12.2 milestone Aug 23, 2024
@jonstacks
Copy link
Collaborator

Thank you for the contribution, @fr6nco!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Issues dealing with the controller area/helm-chart Issues dealing with the helm chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants