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

feat(domains_v1): add support for new UDM endpoints #577

Merged
merged 20 commits into from
Jan 17, 2025

Conversation

Integralist
Copy link
Collaborator

Here are the UDM endpoints being added to go-fastly:
https://www.fastly.com/documentation/reference/api/domains/domain/

Copy link
Member

@tundal45 tundal45 left a 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.

fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1_test.go Outdated Show resolved Hide resolved
fastly/fixtures/domains_v1/cleanup_domains.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@kpfleming kpfleming left a 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.

fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
@Integralist Integralist requested a review from kpfleming January 16, 2025 12:05
@Integralist
Copy link
Collaborator Author

@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.

Copy link
Contributor

@kpfleming kpfleming left a 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.

fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
fastly/domainsV1.go Outdated Show resolved Hide resolved
@Integralist
Copy link
Collaborator Author

@kpfleming the design is looking a lot nicer now 👍🏻

If you could give this another review please. Thanks!

@Integralist Integralist requested a review from kpfleming January 17, 2025 10:54
Copy link
Contributor

@cee-dub cee-dub left a 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. 👍

fastly/domains/v1/api_test.go Show resolved Hide resolved
Copy link
Contributor

@kpfleming kpfleming left a 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!

fastly/domains/v1/api_test.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants