-
Notifications
You must be signed in to change notification settings - Fork 18
Add missing VinylDNS API endpoints #104
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
base: master
Are you sure you want to change the base?
Conversation
Sokitha-21
commented
Jan 21, 2026
- Added endpoint builders and query helpers for health/status, zones, record sets, groups and users.
- Implemented client methods for monitoring, status management, backend IDs, deleted zones, ACL rule add/delete, record set metrics and history, group utilities, and user administration.
- Introduced ownership transfer helpers for record sets (request/approve/reject/cancel) that wrap record set update payloads.
- Added unit tests covering all new endpoints and client methods.
- Fix SigV4 signing for DELETE endpoint.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
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 minor Javadoc discrepancies and some suggestions to make error handling more consistent.
| VinylDNSResponse<GetRecordSetResponse> rsResponse = | ||
| getRecordSet(new GetRecordSetRequest(payload.getZoneId(), payload.getRecordSetId())); | ||
| if (!(rsResponse instanceof VinylDNSSuccessResponse)) { | ||
| throw new RuntimeException("Failed to fetch RecordSet before transfer"); |
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.
Would be better to return a VinylDNSFailureResponse that includes the upstream status/body instead of throwing, to keep error handling consistent for callers.
|
|
||
| http.setEntity(new ByteArrayEntity(payloadBytes, ContentType.APPLICATION_JSON)); | ||
|
|
||
| try (CloseableHttpClient httpClient = |
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.
This DELETE‑with‑body branch builds a new Apache client on each call instead of using the reusable AmazonHttpClient created in the constructor (VinylDNSClientImpl.java:83). That can bypass any configured timeouts/retries/pooling and risks hanging or inconsistent behavior vs other requests.
| VinylDNSResponse<StatusResponse> updateStatus(UpdateStatusRequest request); | ||
|
|
||
| /** | ||
| * Requests ownership transfer for a record seFt. |
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.
Small typo here -> record seFt
| /** | ||
| * Retrieves the list of valid email domains for groups. | ||
| * | ||
| * @return {@link VinylDNSSuccessResponse VinylDNSSuccessResponse<ListValidDomainsResponse>} |
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.
listValidDomains() returns VinylDNSResponse<List>, but Javadoc says ListValidDomainsResponse
| /** | ||
| * Lists available DNS backend IDs that can be used for zones. | ||
| * | ||
| * @return {@link VinylDNSSuccessResponse |
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.
getZonesBackendIds() returns VinylDNSResponse<List>, but Javadoc says GetZonesBackendIdsResponse
| /** | ||
| * Simple health check. | ||
| * | ||
| * @return {@link VinylDNSSuccessResponse VinylDNSSuccessResponse<GetPingResponse>} in case |
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.
getPing() returns VinylDNSResponse, but Javadoc says GetPingResponse
| /** | ||
| * Comprehensive health check including subsystem statuses. | ||
| * | ||
| * @return {@link VinylDNSSuccessResponse VinylDNSSuccessResponse<GetHealthResponse>} in |
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.
getHealth() returns VinylDNSResponse, but Javadoc says GetHealthResponse
| /** | ||
| * Returns current deployment color (blue/green). | ||
| * | ||
| * @return {@link VinylDNSSuccessResponse VinylDNSSuccessResponse<GetColorResponse>} in case |
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.
getColor() returns VinylDNSResponse, but Javadoc says GetColorResponse
| /** | ||
| * Prometheus metrics in text/plain exposition format. | ||
| * | ||
| * @return {@link VinylDNSSuccessResponse |
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.
getPrometheusMetrics() returns VinylDNSResponse, but Javadoc says GetPrometheusMetricsResponse