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

Added swagger doc for GIG DNS API #1017

Merged
merged 3 commits into from
Nov 25, 2023
Merged

Conversation

QiluXie
Copy link
Contributor

@QiluXie QiluXie commented Nov 5, 2023

Problem

We are working with GIG from ITSM to explore the possibility of automatically updating DNS records for agency domains as part of the future site launch process.

As the first step, we are requested to create a swagger API spec to show how the API endpoints could look like before GIG can implement them.

Closes IS-611

Solution

This PR added a swagger file under docs folder.
To view the API specs on Swagger UI, use one of the two methods:

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

@QiluXie QiluXie requested a review from kishore03109 November 5, 2023 13:29
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

Hey! I think a quick win here is make 1 api request pre zone record changed. That would mean for cleaner design as now we dont have to have an entire 4xx error when any one of the requests fail.

I think we could just closely model https://developers.cloudflare.com/api/operations/zones-post here!

The folks did email us, but am worried that flow is really too complicated and conflates business logic within the restful API designs :/ We can discuss this offline

@QiluXie
Copy link
Contributor Author

QiluXie commented Nov 15, 2023

Hey! I think a quick win here is make 1 api request pre zone record changed. That would mean for cleaner design as now we dont have to have an entire 4xx error when any one of the requests fail.

I think we could just closely model https://developers.cloudflare.com/api/operations/zones-post here!

The folks did email us, but am worried that flow is really too complicated and conflates business logic within the restful API designs :/ We can discuss this offline

Hey, sorry, didn't quite get this, why do we want to model cloudflare API to create zone in our case?

@QiluXie QiluXie requested a review from kishore03109 November 15, 2023 15:25
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

Mostly ok, areas for improvement:

  1. Dont understand the priority for the 200 returns, was this intentional?
  2. Could we add a success key in the JSON object?

Concretely, would rather the shape of the return objects to be of:
for

{
"type": "A",
"name": "string",
"content": "string",
"success": true
}

and
{
"type": "A",
"name": "string",
"content": "string",
"success": false
"error": object
}

That way we can expect a similar shape for the return values, agnostic of the success/failure of the operation

@QiluXie
Copy link
Contributor Author

QiluXie commented Nov 24, 2023

Mostly ok, areas for improvement:

  1. Dont understand the priority for the 200 returns, was this intentional?
  2. Could we add a success key in the JSON object?

Concretely, would rather the shape of the return objects to be of: for

{ "type": "A", "name": "string", "content": "string", "success": true }

and { "type": "A", "name": "string", "content": "string", "success": false "error": object }

That way we can expect a similar shape for the return values, agnostic of the success/failure of the operation

Hey, thanks for the comment.

  1. For the priority, if you look at the DNSRecord schema (line 174), you will find it's an optional field under DNSRecord which only applies to MX and SRV records. You can find it on the Cloudflare API doc by selecting MX record for the request body. In this swagger file, it's displaying an example response of the GET DNS records call, therefore it displays all possible optional fields in the schema.
  2. Hmm, I think you might misread the doc on the mentioned response body, it only happens in the GET call which lists the existing DNS records for a given name. Hence there is no failure case in the response for each record. For POST and DELETE call, we do have a 201 and 200 responses indicating the success of the operation and other responses (4xx or 5xx) indicating the failure of it.

Let me know if you want to clarify them over a quick call. Thanks!

@QiluXie QiluXie requested a review from kishore03109 November 24, 2023 07:10
@QiluXie
Copy link
Contributor Author

QiluXie commented Nov 24, 2023

@kishore03109 as discussed, added 401 error response for 3 API calls in ce641d3

@QiluXie QiluXie merged commit bea18c0 into develop Nov 25, 2023
10 checks passed
@mergify mergify bot deleted the doc/IS-611-gig-swagger-doc branch November 25, 2023 13:09
This was referenced Dec 6, 2023
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.

2 participants