-
Notifications
You must be signed in to change notification settings - Fork 0
Add acme-buddy support #34
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #34 +/- ##
==========================================
+ Coverage 97.22% 97.29% +0.06%
==========================================
Files 14 16 +2
Lines 1588 1661 +73
Branches 98 101 +3
==========================================
+ Hits 1544 1616 +72
Misses 35 35
- Partials 9 10 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the main test of this will be in the deployments that use it, as it's difficult to test without a real domain. This repo isn't really set up for integration tests. Or documentation - but I guess the test is a good example to work from..
| self.port = config_integer(data, ["acme_buddy", "port"]) | ||
| self.dns_provider = config_string(data, ["acme_buddy", "dns_provider"]) | ||
| self.env = config_dict(data, ["acme_buddy", "env"]) | ||
| if "ACME_BUDDY_STAGING" in os.environ: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this an env var and not a config value? Is the idea that this allows you to test "real" configs with a fake acme buddy set up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question - in acme buddy, either the --staging flag or existence of a non-empty ACME_BUDDY_STAGING environment var achieve the same purpose - perhaps in constellation I should accept it as optional in both the config, and the environment...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I treat the env part of the config as a block, whatever it contains, so ACME_BUDDY_STAGING can be passed in as acme_buddy: env: ACME_BUDDY_STAGING. If the environment variable is present, then that overwrites the config, which is I think the right way round.
This PR puts the config and container creation code for acme, which we use in many places, into acme.py, so a typical deployment might look like
mrc-ide/packit-deploy#32
essentially calling the constellation functions
acme_cfg = config.config_acme(dat)andacme_container = acme.acme_buddy_container(cfg, proxy, tls_volume)It implements also the suggestions in vimc/montagu-deploy#22 regarding the different environment variables needed for different providers (HDB vs Cloudflare)