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

Create a single variables file for cert-manager + Vault workflow #12

Merged
merged 1 commit into from Jul 6, 2022

Conversation

lonelyCZ
Copy link
Contributor

@lonelyCZ lonelyCZ commented Jul 5, 2022

Signed-off-by: lonelyCZ 531187475@qq.com

Fixes #7

We can configure kubeconfig_path in cm-vault/common.tfvars to apply to all modules.

@lonelyCZ
Copy link
Contributor Author

lonelyCZ commented Jul 5, 2022

/cc @irbekrm

/assign @irbekrm

@irbekrm
Copy link
Contributor

irbekrm commented Jul 5, 2022

Thanks @lonelyCZ !

This looks good to me 👍🏼

Generally it's not a good practice to commit .tfvars files to Git, but I think in this case it should be ok as this is not intended for production use and also I don't think this will ever contain any secrets.

Small nit: could you please update the Readme to mention the new vars file? Apart from that this looks good to merge 👍🏼

@lonelyCZ
Copy link
Contributor Author

lonelyCZ commented Jul 6, 2022

Generally it's not a good practice to commit .tfvars files to Git, but I think in this case it should be ok as this is not intended for production use and also I don't think this will ever contain any secrets.

Yes, this is also my thinking.

Small nit: could you please update the Readme to mention the new vars file? Apart from that this looks good to merge

I have added it, please review it again.

@lonelyCZ lonelyCZ removed their assignment Jul 6, 2022
@lonelyCZ
Copy link
Contributor Author

lonelyCZ commented Jul 6, 2022

/assign @irbekrm

Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thank you for the quick fix @lonelyCZ !

I think the documentation should be in ./cm-vault/README.md not in ./README.md. Apart from that this looks good to go in!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@irbekrm irbekrm assigned lonelyCZ and unassigned irbekrm Jul 6, 2022
Signed-off-by: lonelyCZ <531187475@qq.com>
@lonelyCZ lonelyCZ removed their assignment Jul 6, 2022
@lonelyCZ
Copy link
Contributor Author

lonelyCZ commented Jul 6, 2022

/assign @irbekrm

Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thank you @lonelyCZ !

/lgtm

@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm, lonelyCZ

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot merged commit 0fd7b38 into cert-manager:master Jul 6, 2022
@lonelyCZ lonelyCZ deleted the pr-sigle-vars branch July 26, 2022 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a single variables file for cert-manager + Vault workflow
3 participants