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

feat: Allow Velero container creation #26

Closed
wants to merge 1 commit into from

Conversation

tanmay-pnaik
Copy link
Contributor

@tanmay-pnaik tanmay-pnaik commented Jul 24, 2023

Allow Velero container creation without the use of CRDs

Referred to config at https://github.com/bank-vaults/vault-operator/blob/main/pkg/controller/vault/vault_controller.go

@tanmay-pnaik tanmay-pnaik marked this pull request as ready for review July 24, 2023 17:56
@ramizpolic
Copy link
Member

Great work @tanmay-pnaik, would you mind rebasing with latest changes?
Sorry for a late reply on this, we have been working on features and fixes across other repos.

@tanmay-pnaik tanmay-pnaik requested a review from a team as a code owner November 3, 2023 09:37
@tanmay-pnaik
Copy link
Contributor Author

Great work @tanmay-pnaik, would you mind rebasing with latest changes? Sorry for a late reply on this, we have been working on features and fixes across other repos.

Done!

@akijakya
Copy link
Member

akijakya commented Nov 3, 2023

Great work @tanmay-pnaik, would you mind rebasing with latest changes? Sorry for a late reply on this, we have been working on features and fixes across other repos.

Done!

Hi @tanmay-pnaik, sorry for the inconvenience but merge commits are not allowed on feature branches, could you please remove those and rebase onto main instead? Thanks!

@ramizpolic
Copy link
Member

Can we also move the templates into a separate PR?

Signed-off-by: Tanmay Pereira Naik <[email protected]>
@tanmay-pnaik
Copy link
Contributor Author

Great work @tanmay-pnaik, would you mind rebasing with latest changes? Sorry for a late reply on this, we have been working on features and fixes across other repos.

Done!

Hi @tanmay-pnaik, sorry for the inconvenience but merge commits are not allowed on feature branches, could you please remove those and rebase onto main instead? Thanks!

Done!

@tanmay-pnaik
Copy link
Contributor Author

tanmay-pnaik commented Nov 9, 2023

Great work @tanmay-pnaik, would you mind rebasing with latest changes? Sorry for a late reply on this, we have been working on features and fixes across other repos.

Done!

Hi @tanmay-pnaik, sorry for the inconvenience but merge commits are not allowed on feature branches, could you please remove those and rebase onto main instead? Thanks!

Done!

Can we also move the templates into a separate PR?

Done, see #33

Copy link
Member

@ramizpolic ramizpolic left a comment

Choose a reason for hiding this comment

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

Thanks for the feature addition, but I do have to note something here:

I think we already have a way to enable support for Velero via:

  • .Values.podAnnotations - we can use to inject Velero hook annotations
  • .Values.extraContainers - we can use to add Velero container

I would suggest using these two fields to add this functionality. Therefore, rather than extending the helm chart, perhaps you could document this in the chart readme. Let me know if this would work for you.

@ramizpolic
Copy link
Member

@tanmay-pnaik If it's okay with you, we can close this one. I created #83 to track the other issue. Sorry for the hassle :(

@ramizpolic ramizpolic closed this Dec 22, 2023
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.

3 participants