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

Add cloud sql proxy to admin tools deployment [Issue #263] #259

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

skyfoxs
Copy link

@skyfoxs skyfoxs commented Jan 21, 2022

What was changed

Add Cloud SQL proxy sidecar to the admin tools deployment.

Why?

To make Cloud SQL proxy available to the admin tools pod. So we can use the temporal-cassandra-tool and temporal-sql-tool to connect to the database like the others deployed by server-deployment.yaml and don't have to build it on our machine.

Checklist

  1. Closes
    Close Add cloud sql proxy to admin tools deployment #263

  2. How was this tested:
    Set up and install with sidecar containers as usual. Then shell into admin-tools and connect database with temporal-sql-tool.

  3. Any docs updates needed?
    No

@CLAassistant
Copy link

CLAassistant commented Jan 21, 2022

CLA assistant check
All committers have signed the CLA.

@tsurdilo
Copy link
Contributor

@skyfoxs thanks for the PR!
Could you link it to an existing issue, or open a new one so it can be tracked? Thanks!

@skyfoxs skyfoxs changed the title Add cloud sql proxy to admin tools deployment Add cloud sql proxy to admin tools deployment [Issue #263] Jan 26, 2022
@skyfoxs
Copy link
Author

skyfoxs commented Jan 26, 2022

@tsurdilo done

@tsurdilo
Copy link
Contributor

This looks good to me.
@underrun can you please also take a look? Thanks!

@skyfoxs skyfoxs changed the title Add cloud sql proxy to admin tools deployment [Issue #263] Add cloud sql proxy to admin tools deployment Jan 26, 2022
@skyfoxs skyfoxs changed the title Add cloud sql proxy to admin tools deployment Add cloud sql proxy to admin tools deployment [Issue #263] Jan 26, 2022
@mnichols
Copy link
Contributor

mnichols commented Feb 7, 2022

Just wanted to point out this sidecar support (regardless of whether cloudsqlproxy is the intent) is already in other temporal service helm charts (#107). Maybe that'll help merge it in :)

@kuzmik
Copy link

kuzmik commented Jul 26, 2023

Any chance we can get this merged? I need it myself!

Also the sidecar is needed in the schema-setup jobs, since they require access to cloudsql as well; I'll see about adding that and making a PR for it.

@skyfoxs skyfoxs requested review from a team as code owners July 27, 2023 02:52
@peter-c-larsson
Copy link

Any update on this and did you get around to making PR for the changes @kuzmik ?

@kuzmik
Copy link

kuzmik commented Aug 30, 2023

Any update on this and did you get around to making PR for the changes @kuzmik ?

We worked around it by adding extra sidecars in the jsonnet (we use argo to manage k8s deployments, and the jsonnet repo loads helm charts and adapts them via code).

Originally I had adapted this PR to include the sidecars https://github.com/temporalio/helm-charts/pull/259/files; but since we generally prefer not to modify the helm charts in our repo if we can avoid it, I converted it to the sonnet stuff.

local k = import 'ksonnet-util/kausal.libsonnet';

(import './config.libsonnet')
+ {
  local c = $._config.temporal,
  local deployment = k.apps.v1.deployment,

  temporal+: {
    deployment_temporal_admintools+:
      deployment.spec.template.spec.withContainersMixin(c.containerCSP)
      + deployment.spec.withMinReadySeconds(10)
      + c.mysqlCSISecretMount,

for example.

@robholland
Copy link
Contributor

Please adjust this to add and use admintools.sidecarContainers instead.

@robholland robholland added enhancement New feature or request needs revision Team has requested some changes labels Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs revision Team has requested some changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add cloud sql proxy to admin tools deployment
7 participants