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_separate_group.db_secret #231

Merged
merged 1 commit into from
Nov 17, 2024

Conversation

vladimir4862
Copy link
Contributor

Hello!
I'm not very experienced with Trino configuration, but I do have a bit more experience with Helm charts.
The main question in this PR is determining the correct place to change the file.password-file and file.group-file configurations.

I placed them in a separate folder to keep automatic updates when k8s secrets are updated.
This approach contrasts with using subPath, which does not support automatic file updates within pod volumes.

@cla-bot cla-bot bot added the cla-signed label Sep 27, 2024
@vladimir4862
Copy link
Contributor Author

I was inspired by the #188
So maybe @robertpikmets also whant to look at this.

Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

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

This looks good, but it needs a test. Look at test.sh and add a new test case with a separate values file. Thanks for working on this!

charts/trino/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/trino/templates/configmap-coordinator.yaml Outdated Show resolved Hide resolved
@vladimir4862
Copy link
Contributor Author

I'll check and make test during this week

@vladimir4862
Copy link
Contributor Author

I changed group to goups almost in all cases.
to keep the same world in template and values.

I still not sure what changes, and test cases you are talking about
testCase "complete_values" checked this changes. because this value has groups. https://github.com/trinodb/charts/blob/main/test-values.yaml#L37

Could you provide more information about it? I really want to help here, but not sure how.

charts/trino/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/trino/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/trino/templates/secret.yaml Outdated Show resolved Hide resolved
@vladimir4862
Copy link
Contributor Author

Terrably sorry for this little typos. 😸
just fixed all.
about naming, As I said i'm not very familiar with trinodb itself, just whant to add a some feature in helm chart which will help me to deploy it.

@nineinchnick
Copy link
Member

As the old joke says, the two hardest problems in IT are:

  1. naming
  2. cache invalidation
  3. off-by-one errors

Let's figure out good names here together.

@nineinchnick nineinchnick added the enhancement New feature or request label Oct 30, 2024
@nineinchnick
Copy link
Member

@cla-bot check

Copy link

cla-bot bot commented Nov 17, 2024

The cla-bot has been summoned, and re-checked this pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants