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

Use existing secret rework #4

Merged
merged 4 commits into from
Nov 3, 2023
Merged

Conversation

mrdotb
Copy link
Contributor

@mrdotb mrdotb commented Nov 2, 2023

What this PR does / why we need it:

Use existing secret.

Changes:

I added the ability to bring an existing secrets that we discussed in #3.

I separated the values related to postgres and clickhouse optional deps to be first level databaseURL, clickhouseDatabaseURL
We can discuss this but I feel like it's confusing to access values from deps that are not supported in their chart and if you want to bring your own postgres or clickhouse it make more sense.

I got ride of the secrets postgres and clickhouse password ... that are only used in the init containers and replaced that by parsing the DB URL in scripts instead. I also moved the init container check on first level for the same reasons as before.
I also got ride of the clickhouse curl check not sure why two scripts are needed ?

I updated the plausible ENV some where removed some where added. (twitter, adminUser, mailer ...)

The chart version need to be bumped but I will do that after the review.

There is an issue with the CI does not seem related with my changes ERROR: Unable to validate cosign version: 'v2.0.0'

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md

Copy link
Member

@alexnuttinck alexnuttinck left a comment

Choose a reason for hiding this comment

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

Hi @mrdotb,

Thanks a lot for your contribution and the refactoring about the environment variables and the init scripts!
Could you have a check about my 2 comments before we can merge?

I also got ride of the clickhouse curl check not sure why two scripts are needed ?

I added this init check because we use an external clickhouse where the port 9000 is not opened. But I agree there is no sense to have 2 methods to verify that clickhouse is ready.

The chart version need to be bumped but I will do that after the review.
I will bump it to 0.2.0 as a lot of default variables have changed.

There is an issue with the CI does not seem related with my changes ERROR: Unable to validate cosign version: 'v2.0.0'

I will have a look. It seems to be related to this isuue.

{{- if .Values.postgresql.url }}
DATABASE_URL: {{ .Values.postgresql.url | toString | b64enc }}
{{- if .Values.mailer.enabled }}
{{- if eq .Values.mailer.adapter "Bamboo.SMTPAdapter" }}
Copy link
Member

Choose a reason for hiding this comment

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

@mrdotb I think we should be able to set the SMTP_USER_PWD for any mailer adapters.

Copy link
Contributor Author

@mrdotb mrdotb Nov 3, 2023

Choose a reason for hiding this comment

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

Why for ?
The https://hexdocs.pm/bamboo_smtp/Bamboo.SMTPAdapter.html use smtp but all the others rely on third party API that only need an api key except mailgun.
The only common things to mailer are the email which is the sender.
My update on mailer values reflect that.

mailer:
  enabled: false  # Enable/Disable functionality
  email: ""  # the email address of the email sender
  adapter: ""  # "Bamboo.SMTPAdapter", "Bamboo.MailgunAdapter", "Bamboo.MandrillAdapter", "Bamboo.SendGridAdapter"
  smtp:
    host: ""  # The host address of your smtp server.
    port: ""  # The port of your smtp server.
    username: ""  # The username/email in case SMTP auth is enabled.
    password: ""  # The password in case SMTP auth is enabled.
    ssl: ""  # If SSL is enabled for SMTP connection
    retries: ""  # Number of retries to make until mailer gives up.
  mailgun:
    apiKey: ""
    domain: ""
    baseURI: ""
  postmarkApiKey: ""
  mandrillApiKey: ""
  sendgridApiKey: ""

Did I miss something ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/pablo-co/bamboo_postmark for reference only require the postmarkApiKey

Copy link
Member

Choose a reason for hiding this comment

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

@mrdotb Nope sorry, I misunderstood when I read the docs from plausible. Thanks for the link, you are right! Let's merge 🚀

{{- end }}
{{- if .Values.smtp.host }}
{{- if eq .Values.mailer.adapter "Bamboo.SMTPAdapter" }}
Copy link
Member

Choose a reason for hiding this comment

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

@mrdotb the same here, I think we should be able to set the SMTP_HOST_ADDR for any mailer adapters.

@alexnuttinck alexnuttinck merged commit 0729096 into IMIO:main Nov 3, 2023
1 check passed
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