Skip to content

Conversation

@dberardo-com
Copy link

closes #64

@phlg
Copy link
Contributor

phlg commented Feb 28, 2024

Hello, and sorry for the awful delay on replying to your issue and PR. I'll also address the issue you opened on the s3-operator repo proper soon.

Your PR seems to go beyond the scope of #64. If I'm not mistaken, you're also :

  • adding the missing bits that prevented caCertificateBundlePath from being usable
  • adding an extra parameter to manage the --useSsl flag

Everything is welcome, though I still have some questions and comments for clarification :

  • Any specific reason you used a secret rather than a CM to manage de CA Bundle ? Not that I'm asking to change, this is more out curiosity (I would usually treat this as non sensitive information, but I may be wrong in the regard)
  • For the --useSsl problem, another contributor also tackled the problem, with a more generic solution (see Extra args and env #56). Hopefully, we should be able to merge his PR soon, in which case could you remove that bit from your own branch ?

As with #56, we should be able to merge your PR with some minor tweaks :

  • bump the chart version to next minor in Chart.yaml
  • remove the bit pertaining to --useSsl (which should be covered by Extra args and env #56)
  • rebase over latest master

Thank you for the contribution !

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.

add support for different secret key names

2 participants