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

Metabase: Google Cloud SQL support #22

Merged
merged 3 commits into from
Nov 8, 2021
Merged

Metabase: Google Cloud SQL support #22

merged 3 commits into from
Nov 8, 2021

Conversation

tclift
Copy link

@tclift tclift commented Nov 4, 2021

Add support for Google Cloud SQL through use of the Cloud SQL Auth proxy running as a sidecar.

This method supports the recommended Workload Identity method of authentication. It does not support using a specific service account credentials (JSON) file.

In case specific database provider support is not desirable in this chart, an alternative approach would be to allow arbitrary additional container support in the Deployment via values.yaml (perhaps along with an example for how to get Cloud SQL working with that approach).

This allows Metabase to use Google Cloud SQL for its internal database,
or for one or more additional databases. When enabled, the
`gcr.io/cloudsql-docker/gce-proxy` image is deployed as a sidecar in the
Deployment.
@pmint93
Copy link
Owner

pmint93 commented Nov 5, 2021

Great feature for GKE users 👍

Please also bump the chart version in Chart.yaml so it pass the checks and we can merge this PR

Update Metabase chart version from 1.2.0 to 1.3.0 for feature addition:
Google Cloud SQL database support.
@tclift
Copy link
Author

tclift commented Nov 5, 2021

Thanks, chart version bumped.

By the way, I tested this by running chart-releaser-action locally using act, with the resulting release being uploaded to my fork. It might have been specific to the act environment, but I hit a bug with uses: helm/[email protected] (can't recall the details now). I upgraded it to 1.2.1 and hit this bug, which might be specific to using an SSH remote. Downgrading to 1.2.0 worked.

Which is to say... you should be safe to upgrade chart-releaser-action to 1.2.0 if you wanted :)

@pmint93
Copy link
Owner

pmint93 commented Nov 8, 2021

@tclift Thanks a lot for your detailed information.

Btw, the test currently failed due to "missing starting space in comment" in values.yaml:68

@tclift
Copy link
Author

tclift commented Nov 8, 2021

Sorry about that. I've fixed it up.

FWIW, I disagree with the linter. Space-after-hash is fine for human readable comments, but the line in question was a commented out option. The user should be able to remove the hash to enable the option, without being concerned about indentation. I can see now that other options in the file have a space-after-hash for the same thing... it does seem like capitulation to an unhelpful linting rule.

I also worked out how to get the linter running locally:

docker run -it --rm -v `pwd`:/repo -w /repo quay.io/helmpack/chart-testing:latest ct lint

(this might be useful for the README or CONTRIBUTING)

@pmint93
Copy link
Owner

pmint93 commented Nov 8, 2021

I agree with you that space-after-hash is just fine for human readable. But I think the reason for that is people usually use IDE, code editor shortcut to commenting out like Ctrl ( or Command) + / so it automatically add/remove a hash and a space

@pmint93 pmint93 merged commit 648f588 into pmint93:master Nov 8, 2021
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