-
Notifications
You must be signed in to change notification settings - Fork 597
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
Support for attaching a Worker to a Domain #1014
Support for attaching a Worker to a Domain #1014
Conversation
changelog detected ✅ |
Codecov Report
@@ Coverage Diff @@
## master #1014 +/- ##
==========================================
+ Coverage 49.06% 49.72% +0.66%
==========================================
Files 108 115 +7
Lines 10428 10862 +434
==========================================
+ Hits 5116 5401 +285
- Misses 4200 4299 +99
- Partials 1112 1162 +50
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
a2c37b8
to
ff5e502
Compare
Working on tests next. @jacobbednarz should I switch the arguments to be inside a struct? Some of the methods use a strict while others take direct arguments. I'm leaning towards switching to the struct pattern. Let me know! |
check out https://github.com/cloudflare/cloudflare-go/blob/master/docs/experimental.md, #1028, and https://github.com/cloudflare/cloudflare-go/blob/master/tunnel.go for prior art on how we should be adding new methods. |
@jacobbednarz Updated per your examples, please review! It seems like In workers.go, I followed the example of DeleteWorker, DownloadWorker, etc. that have ZoneID in the respective params structs. |
there are currently no other implementation of going forward, we want to use the updated method signature for all new methods to help future proof against these issues. |
Just to be clear, does that mean I should add the new method using the new style in a manner differently than my latest commit, or that you'll circle back to workers.go at a later date and redo all the method signatures in one go? PS - I would expect to squash all commits in this PR, let me know if you'd like to do that as part of a review and merge or if I should force push my branch. I am very comfortable with rebase workflow, so this is no hardship at my end. |
sorry if it wasn't clear. you should do this (i'll add a note inline as a reminder).
no need, i'm happy to merge/squash at the end. |
workers_test.go
Outdated
attachWorkerToDomainResponse = `{ | ||
"result": { | ||
"id": "e7a57d8746e74ae49c25994dadb421b1", | ||
"zone_id":"foo", |
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.
you'll want to use testZoneID
here instead.
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.
Need to move out of the const block to the var block in order to use fmt.Sprintf here
9f54d25
to
ee5e98e
Compare
thanks! appreciate the effort here! |
Thank you! |
This functionality has been released in v0.47.0. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
Description
Support for the new Custom Domain API at https://api.cloudflare.com/#worker-domain-attach-to-domain
Has your change been tested?
Works for me locally, unit tests will be added in PR review.
Screenshots (if appropriate):
Types of changes
What sort of change does your code introduce/modify?
Checklist: