-
Notifications
You must be signed in to change notification settings - Fork 128
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
feat(domains_v1): add support for new UDM endpoints #577
Conversation
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.
Lots of questions all of which are for my edification and non-blockers. I have one nit which is also not a blocker.
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've left a number of suggestions, and requested that plural operation names be changed to singular when they operate on a single domain.
Now, a more radical suggestion: I'm really not a fan of these operations being labeled "DomainsV1" :-) I've recently added the first operations in go-fastly which are not in the fastly
namespace directly (they are below it). A similar thing could be done here, locating these operations at fastly/domains/v1
, and then removing DomainsV1
from all the structure and function names to simplify them.
I think this will be easier for users to understand, but it's definitely not mandatory. Moving things out of fastly
also helps with testing as you can't accidentally rely on things that are unexported (which I ran into while working on fastly/products
.
Co-authored-by: Kevin P. Fleming <[email protected]>
Co-authored-by: Kevin P. Fleming <[email protected]>
Co-authored-by: Kevin P. Fleming <[email protected]>
Co-authored-by: Kevin P. Fleming <[email protected]>
Co-authored-by: Kevin P. Fleming <[email protected]>
Co-authored-by: Kevin P. Fleming <[email protected]>
Co-authored-by: Kevin P. Fleming <[email protected]>
Co-authored-by: Kevin P. Fleming <[email protected]>
Co-authored-by: Kevin P. Fleming <[email protected]>
Co-authored-by: Kevin P. Fleming <[email protected]>
@kpfleming I've made the changes you've suggested. I'm just going through the more radical change you suggested, to see if it's something I can turn-around in a reasonable time frame (as we're bit strained for time as far as getting this work done and so now might not be the time to do that change about). I'll let you know later how I get on. |
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.
Just a few stragglers in comments, otherwise this is fine.
@kpfleming the design is looking a lot nicer now 👍🏻 If you could give this another review please. Thanks! |
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.
Looks clean in the v1 directory. 👍
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.
One minor change, otherwise this is good to go. It looks great!
Here are the UDM endpoints being added to go-fastly:
https://www.fastly.com/documentation/reference/api/domains/domain/