-
Notifications
You must be signed in to change notification settings - Fork 16.7k
Conversation
Hi @grugnog. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign |
Please compare with what you get with |
@unguiculus I think this should be ready for further review. |
@unguiculus did you get a chance to look at the revisions? |
*/}} | ||
{{- define "redash.fullname" -}} | ||
{{- if .Values.fullnameOverride -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If fullnameOverride
is set, you get the same for redash.fullname
, redash.adhocWorker.fullname
, etc. I guess this is not intended and will cause problems/name clashes.
@@ -0,0 +1,57 @@ | |||
apiVersion: extensions/v1beta1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apps/v1beta2
@@ -0,0 +1,57 @@ | |||
apiVersion: extensions/v1beta1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apps/v1beta2
@@ -0,0 +1,120 @@ | |||
apiVersion: extensions/v1beta1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apps/v1beta2
@@ -37,8 +37,8 @@ We truncate at 63 chars because some Kubernetes name fields are limited to this | |||
If release name contains chart name it will be used as a full name. | |||
*/}} | |||
{{- define "redash.adhocWorker.fullname" -}} | |||
{{- if .Values.fullnameOverride -}} | |||
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}} | |||
{{- if .Values.adhocWorker.fullnameOverride -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need separate fullnameOverride for the components and configurable component names? How about doing something like this:
{{- define "redash.adhocWorker.fullname" -}}
{{- template "redash.fullname" . -}}-adhoc-worker
{{- end -}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@unguiculus I don't need this personally - I was trying mainly to keep things consistent with the Helm 2.8+ create template and also copying the pattern used by some of the other multi-deployment charts like Prometheus.
If you think it's cleaner to drop fullnameOverride I am happy to remove it. If so let me know if you think fullnameOverride should be removed from all deployments or kept on the primary/web deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest you follow the example above. You'll have one fullnameOverride affecting all derived fullnames.
can we get this finished and merged.. It would be fabulous. |
@unguiculus I think this should be ready for further review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest you move the chart to stable before we merge.
incubator/redash/values.yaml
Outdated
imageTag: "9.5.6-alpine" | ||
postgresUser: redash | ||
postgresPassword: redash | ||
postgresDatabase: redash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't set a default value for the password
incubator/redash/values.yaml
Outdated
## ref: https://github.com/kubernetes/charts/blob/master/stable/redis/README.md | ||
## | ||
redis: | ||
redisPassword: redash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't set a default value for the password
incubator/redash/values.yaml
Outdated
## This PostgreSQL instance is used for all Redash state storage | ||
## ref: https://github.com/kubernetes/charts/blob/master/stable/postgresql/README.md | ||
## | ||
postgresql: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to define an external postgresql database instead of running its own? for setups that already have a DB running (i.e. using AWS managed service) this would remove the need for persistence too.
@grugnog @unguiculus Hi, are there plans to merge this anytime soon? It would be great to have a redash helm chart available in the stable repo. |
this is really helpful, I've been using this for some time now. it's pretty stable |
Hey, i think redash plans to release the v5 sometime in the near future. I hope to be using the v5 on kubernetes when it's released. I have an working v4 not on kubernetes, was about to migrate to kubernetes, but then i saw something about the update so I am going to wait for it. I am willing to do some work on the v5 for kubernetes if needed! |
I don't think there should be anything in this Chart that should change (aside from the Docker image reference) between v4 and v5. Hence no point in waiting for v5 to finalize or merge this. |
@arikfr great!! I didn't take a look on redash v5 upgrade neither this chart yet, so i wouldn't know, i just added my 2c! |
Sorry for the delay in getting back to this. I have moved to stable and also updated the image tag. Avoiding default passwords is a bit complex as Redash expects the password as part of connection URL strings (REDASH_DATABASE_URL and REDIS_URL). The autogenerated passwords are in a secret and not accessible during templating, so we would need to inject a script via Configmap to assemble each URL environment variable as part of the container startup from the parts (hostname, password etc). Alternatively we could see if Redash would be able to update the Docker image to accept the URL components instead of just a single URL string? |
I am working on a change to split out DB env variables and merge them in a shell script loader (that could hopefully eventually become part of the upstream entrypoint). This should allow Redash to be configured to use the autogenerated. It's still a WIP, but at https://github.com/grugnog/charts/commits/redash-env if you want to take a look at the direction. |
@grugnog are you still working on this? By the way, version 5 is released, you may use it! Let me know if not, so I will see if I have time take over or not! |
@grugnog Thanks for putting this together; super helpful! Any estimate on when you think it'll land? I'd love to give it a whirl. Also any particular reason 'scheduledworker-deployment..yaml' has a double period in the file name? I've not seen that before. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: grugnog The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
DCO is signed (sorry for delay - just got back from vacation!). |
Signed-off-by: Owen Barton <owen.barton@civicactions.com>
@unguiculus ping 🥇 |
/ok-to-test |
please take a look at helm/helm#6095 (comment) we strongly encourage people to self host chart repos: you can even do that using GitHub pages: |
@grugnog : Hey, I would like to request some changes which we did to our vendored chart from this PR inside our organization. Please consider adding the following under
In values.yaml you can provide the following default values:
This way users can use existing secret inside the cluster the secret key that is used in newer versions for redash. See https://version.redash.io/ . This is required for us running with redash7. Here is our internal upgrade transcript we logged when we did our upgrade from redash5 to redash7 below. Maybe it could be useful as a contrib readme or something. How to upgrade Redash (database migration)Example transcript below, in the following example redash5 is old deployment, Scale all worker deployments down...but leave postgresql/redis running:
Create database dumppg_dump -Fc -U redash redash > /tmp/2019-08-09-upgrade-redash-compressed.backup Scale old deployment up again
But instruct #general in slack that new edits in queries will not be included Upgrade chartSince its not merged to upstream yet ( #5071 git clone https://github.com/grugnog/charts.git
git checkout redash
(cd stable/redash && helm dependency update)
mkdir /tmp/new-redash
helm package stable/redash --destination /tmp/new-redash
tar -C /tmp/new-redash -zxvf /tmp/new-redash/redash-1.0.0.tgz
mv /tmp/new-redash/redash ~/zedge/deployments/charts/ Ensure we use chart values which refer to correct secrets, As of upgrading redash7, redash chart expects specific naming on After chart upgrade in a new Helm releaseWe sadly get a new postgresql instance. Scale down new deployment while we restore database contents.
Restore databaseCopy dump back in kubectl cp ~/tmp/2019-08-09-upgrade-redash-compressed.backup redash7-postgresql-0:/tmp/2019-08-09-upgrade-redash-compressed.backup Enter postgresql pod kubectl exec -ti redash7-postgresql-0 /bin/bash -i Restore contents: pg_restore -h localhost -p 5432 -U redash -d redash --clean /tmp/2019-08-09-upgrade-redash-compressed.backup (You can ignore the comment about it not being allowed to set a comment on a SQL procedure)
kubectl scale deploy/redash7 --replicas=1
The /config/dynamicenv.sh sets the required variables for using Success?!Visit https://redash.example.net (depends if you use release name in helm set to redash. Other useful tricksRemember to have a shell which have inherited the /config/dynamicenv.sh ! Check the current version and migration history:/app/bin/docker-entrypoint manage db current
/app/bin/docker-entrypoint manage db history See the SQL commands that will be run:/app/bin/docker-entrypoint manage db upgrade --sql Migrate/app/bin/docker-entrypoint manage db upgrade OAuth configurationConfiguration towards Google OAuth is done in GCP Google OAuth project, you (This section about oauth configuration you could drop for this contrib upgrade readme if you want to include it) |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions. |
nono dont stale this, merge it! |
@zakkg3 This is a long thread, so let me summarize the bad news... It looks like this PR will not be merged because:
from #5071 (comment) and a more concise take, from another PR: linkerd/linkerd2#3292 (comment) So I'll open an issue on getredash/redash asking if they'd like to own the helm chart. In the past they've had kube docs and scripts, so I think it is likely they will want to host the helm cart under the getredash org. If they are not interested we could submit @grugnog's repo to chart hub. I assume we'll need to cleanup that repo. |
Anyone who likes or is following this thread, please vote on the Redash feature discussion: |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions. |
@arikfr @yzorg - thanks so much for making this connection and establishing a new repo part of the getredash org. I have pushed a rebased version of this PR to that repository and opened a handful of initial tickets. Let's continue work there - PRs are very welcome :D |
The end of the big story... |
What this PR does / why we need it:
This adds a Redash chart.
Redash is an open source collaborative data analytics and visualization tool.
Special notes for your reviewer:
A version of this chart has been running without issue in an internal context for several weeks.
Note that Redash is developed by a startup of the same name. I am not affiliated with that company, but I will open an issue on their Github tracker to make them aware of this PR and see if they have input or interest in co-maintaining.