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

Updated list ipv6 response #762

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

imaskm
Copy link
Contributor

@imaskm imaskm commented Nov 27, 2024

No description provided.

Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep this consistent with existing List methods. For example:

func (r *ReservedIPsServiceOp) List(ctx context.Context, opt *ListOptions) ([]ReservedIP, *Response, error) {

We return a slice of the resource while making the pagination info accessible via the Response

godo/godo.go

Lines 165 to 170 in 9338200

// Links that were returned with the response. These are parsed from
// request body and not the header.
Links *Links
// Meta describes generic information about the response.
Meta *Meta

@@ -34,6 +34,15 @@ type ReservedIPV6 struct {
ReservedAt time.Time `json:"reserved_at"`
Droplet *Droplet `json:"droplet,omitempty"`
}
type ReservedIPV6Resp struct {
ReservedIPV6 *ReservedIPV6 `json:"reserved_ipv6"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Hey Ashwani, Thanks for the PR 🚀.

Here are some suggestions you may consider:

  1. Could you please add Links to the ReservedIPV6Resp as it is done here? This will require some changes in Create. You can check reserved ips for reference.

  2. Could you also rename ReservedIPV6Resp to reservedIPV6Root for consistency with existing code? 🙏

  3. Let's return *ReservedIPV6 instead of *ReservedIPV6Resp for consistency with existing code? 🙏

  4. Let's try to reuse reservedIPV6Root in List as it is done here

Please, let me know when you are done. I'll be happy to take a look again 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loosla

  1. We don't have any Links in Create and Get response for IPv6 API.
  2. Ok but *Root didn't make sense
  3. Ok
  4. for list it's plural reservedIPsRoot is being in IPv4

@loosla
Copy link
Contributor

loosla commented Nov 27, 2024

This comment is going to be out of scope of this PR, but it is something we should consider.

Would it be possible to add API documentation for a new /v2/reserved_ipv6 at some point?
This would showcase the fantastic work you’ve done in our publicly available documentation and also help simplify the review process. 🙏

API documentation repo is here - https://github.com/digitalocean/openapi

Many thanks.

@imaskm imaskm requested a review from loosla November 27, 2024 18:51
@imaskm
Copy link
Contributor Author

imaskm commented Nov 27, 2024

This comment is going to be out of scope of this PR, but it is something we should consider.

Would it be possible to add API documentation for a new /v2/reserved_ipv6 at some point? This would showcase the fantastic work you’ve done in our publicly available documentation and also help simplify the review process. 🙏

API documentation repo is here - https://github.com/digitalocean/openapi

Many thanks.

There is a PR for this: digitalocean/openapi#951

Copy link
Contributor

@loosla loosla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me if it aligns with all changes in digitalocean/openapi#951 and that's ok that Links are missing in Create.

Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 Thanks for the changes!

@loosla loosla merged commit a97e397 into digitalocean:main Nov 27, 2024
8 checks passed
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.

3 participants