Version 0.8 - API integration #25
Conversation
There was a problem hiding this comment.
I didn't get to run it locally just yet but keen on doing so before the end of the week if you're not in a hurry to merge.
FWIW, this implementation won't be compatible with multi-sites builds (unless they are served as sub-folders on a same domain), but I don't think the old way was compatible either and, to be honest, multi-sites builds are rather niche and a bit flaky with wagtail-bakery so there are some upstream work to do before we can worry of that ourselves.
|
|
||
| API_ROOT = "https://api.netlify.com/api/v1/" | ||
| HEADERS = { | ||
| "Authorization": str("Bearer " + settings.NETLIFY_API_TOKEN), |
There was a problem hiding this comment.
Why is there a cast to str here? If the fear is that settings.NETLIFY_API_TOKEN isn't a string already, then it would still fail because the concatenation would have happened already and failed with a ValueError. The cast to str should be around settings.NETLIFY_API_TOKEN, or it could use an f-string which would do the casting automatically.
There was a problem hiding this comment.
Actually, NETLIFY_API_TOKEN being an optional parameter, we should account for that and raise an error for trying to use the API without authentication, giving a clear indication of the error rather than having to parse Netlify's response.
This also applies to NETLIFY_SITE_ID in get_site_name and netlify_deploys below.
That didn't age well. I'm not sure when I'll have time to look it up again so don't let me stop you from merging. |
There was a problem hiding this comment.
So far it looks OK as it is. @tomdyson could address @loicteixeira and my comments before approving this PR.
| "deploy", | ||
| "--dir={}".format(settings.BUILD_DIR), | ||
| "--prod", | ||
| '--message="Wagtail Deployment"', |
This is a larger than usual update, with backwards-incompatible changes.
Deploymentmodel has been removed, in favour of direct integration with the Netlify API