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

Secrets consistency for the chart #1016

Merged
merged 2 commits into from
Jun 23, 2023
Merged

Conversation

ksa-real
Copy link
Contributor

@ksa-real ksa-real commented Dec 17, 2022

  • Enabling existing secrets for external MySQL and Redis
  • Tolerate existing secrets for bundled charts.
  • README.md: secrets handling explained.
  • Fixed multiple bugs where missing required field was replaced with default instead of failing.
  • PHONE_NOTIFICATIONS_LIMIT was on the wrong level: it was not set if existingSecret was true.

Next are the cosmetic changes. They improve chart consistency, e.g. prevent generation of multiple new lines in certain cases:

  • Common approach to spaces trimming. This typically allows curly blocks and actual strings indentation and nice nindent usage:

    • Two curly blocks should not trim the same space. I.e. "{{ ... -}} {{- ... }}" shouldn't happen.

    • Template generates either single line or multiline string. In both cases, no new line appears on both sides of the output string. So we delete unnecessary new lines inside and at the end of string with "trim-to-left" ({{- ) and the leading new line using "trim-to-right" (-}}).
      Note that trimming both leading and trailing new line is not always easily possible: Function similar to nindent but ignoring content's initial new line. Masterminds/sprig#357

      Example.

      {{- define "mytemplate" -}}
      {{ if someBoolean -}}
        {{ .Value.some }}
      {{- else -}}
        some string
      {{- end }}
      {{- end }}
      
  • template replaced with include. It is often recommended to use include by default, as it allows pipelining.

Checklist

  • Tests updated - No tests for Helm chart
  • Documentation added
  • CHANGELOG.md updated

@ksa-real ksa-real requested a review from a team December 17, 2022 07:20
@ksa-real ksa-real force-pushed the existing-secrets branch 3 times, most recently from 2d9a3ce to b66463f Compare December 17, 2022 08:25
@matiasb
Copy link
Contributor

matiasb commented Dec 19, 2022

At first sight, suggestions make sense to me. @iskhakov, would you take a look?

@ksa-real ksa-real force-pushed the existing-secrets branch 2 times, most recently from b44ac2f to ad738a1 Compare December 20, 2022 00:23
@ksa-real
Copy link
Contributor Author

@iskhakov Friendly ping

@marcusteixeira
Copy link

Ac

@ksa-real
Copy link
Contributor Author

Can someone take a look, please?

@ksa-real
Copy link
Contributor Author

Ac

Is it a common abbreviation? I wonder what it means in this context :)

@ksa-real ksa-real force-pushed the existing-secrets branch 2 times, most recently from 2db0aeb to d8d3a5a Compare February 10, 2023 23:07
@ksa-real
Copy link
Contributor Author

@Matvey-Kuk @iskhakov Can you please assign this to someone?

@ksa-real
Copy link
Contributor Author

ksa-real commented Mar 3, 2023

Closed accidentally by pushing a version with incomplete rebase

@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2023

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the Stale label Jun 8, 2023
@iskhakov iskhakov removed their assignment Jun 19, 2023
Copy link
Contributor

@iskhakov iskhakov left a comment

Choose a reason for hiding this comment

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

waiting for changes to pass tests

@ksa-real ksa-real force-pushed the existing-secrets branch 2 times, most recently from cb3ba35 to ad3cebf Compare June 19, 2023 20:33
@ksa-real
Copy link
Contributor Author

I have doubts about TWILIO_ACCOUNT_SID. IMO it should reside in existingSecret if such exists (as being a part of credentials).

@ksa-real ksa-real force-pushed the existing-secrets branch 6 times, most recently from 269b402 to d8f93e3 Compare June 19, 2023 22:02
@ksa-real
Copy link
Contributor Author

ksa-real commented Jun 20, 2023

@iskhakov Can you please approve the workflows? Note, that due to a single file accidental change (despite the later reversal) a bunch of unnecessary tests were triggered. Not sure if it is possible to remove them, or we should just run the unnecessary tests.

@ksa-real
Copy link
Contributor Author

@iskhakov Please, re-approve the workflows

@ksa-real ksa-real force-pushed the existing-secrets branch 6 times, most recently from f409dc9 to 55462d7 Compare June 22, 2023 10:41
- Enabling existing secrets for external MySQL and Redis
- Tolerate existing secrets for bundled charts.
- README.md: secrets handling explained.
- Fixed multiple bugs where missing required field was replaced with
  default instead of failing.
- PHONE_NOTIFICATIONS_LIMIT was on the wrong level: it was not set if
  existingSecret was true.

Next are the cosmetic changes. They improve chart consistency, e.g.
prevent generation of multiple new lines in certain cases:
- Common approach to spaces trimming. This typically allows curly blocks
  and actual strings indentation and nice `nindent` usage:
  - Two curly blocks should not trim the same space. I.e.
    "{{ ... -}} {{- ... }}" shouldn't happen.
  - Template generates either single line or multiline string. In both
    cases, no new line appears on both sides of the output string. So we
    delete unnecessary new lines inside and at the end of string with
    "trim-to-left" (`{{-` ) and the leading new line using
    "trim-to-right" (`-}}`).
    Note that trimming both leading and trailing new line is not
    always easily possible:
    Masterminds/sprig#357

    Example.

    ```
    {{- define "mytemplate" -}}
    {{ if someBoolean -}}
      {{ .Value.some }}
    {{- else -}}
      some string
    {{- end }}
    {{- end }}
    ```

- `template` replaced with `include`. It is often recommended to use
  `include` by default, as it allows pipelining.
@ksa-real
Copy link
Contributor Author

I've fixed the tests. Everything seems to run fine in my forked workflow, except for e2e tests which are slow to be picked up by workers: workflow

@iskhakov iskhakov added this pull request to the merge queue Jun 23, 2023
Merged via the queue into grafana:dev with commit 370d7b9 Jun 23, 2023
@ksa-real ksa-real deleted the existing-secrets branch June 23, 2023 06:24
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.

4 participants