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

Enable policies for cert-manager. #885

Merged
merged 2 commits into from Jun 17, 2019

Conversation

@Yannig
Copy link
Contributor

commented Jun 14, 2019

Fix for #745

Description

Checklist

  • Code compiles correctly (i.e make build)
  • Added tests that cover your change (if possible)
  • All unit tests passing (i.e. make test)
  • All integration tests passing (i.e. make integration-test)
  • Added/modified documentation as required (such as the README.md, and examples directory)
  • Added yourself to the humans.txt file
@errordeveloper

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@Yannig thanks very much! This looks good overall, but I wonder if we should make this controlled by a dedicated config field instead of hanging it on externalDNS – what do you think?

@Yannig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@errordeveloper no problem. I'm doing it asap.

@Yannig Yannig force-pushed the Yannig:cert-manager branch from 16a276b to 1ac1bc6 Jun 14, 2019

@Yannig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@errordeveloper what do you think about it?

@errordeveloper

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@Yannig thank you, this looks good! Ideally, we would make this a config-only option, if you are okay with that? We have a strong preference to avoid adding new flags, as there are already too many.

@Yannig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

I understand. I'm doing the modification.

@Yannig Yannig force-pushed the Yannig:cert-manager branch 2 times, most recently from e35dc29 to a60ea29 Jun 17, 2019

@Yannig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

@errordeveloper I think it's fine now. What do you think about?

@errordeveloper
Copy link
Member

left a comment

This looks good! If you have a moment to replace new PolicyExternalDNS strings with PolicyCertManager, it would be great, otherwise I'm happy to follow up.

Add new certManager config option.
This options adds needed policies to be able to deploy a certificate manager.

@Yannig Yannig force-pushed the Yannig:cert-manager branch from a60ea29 to 2e0924a Jun 17, 2019

@Yannig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Done

@errordeveloper errordeveloper merged commit 2bc4479 into weaveworks:master Jun 17, 2019

3 checks passed

WIP Ready for review
Details
ci/circleci: make-eksctl-image Your tests passed on CircleCI!
Details
netlify/brave-ritchie-0c2cd8/deploy-preview Deploy preview ready!
Details

@Yannig Yannig deleted the Yannig:cert-manager branch Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.