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(api domain): extend options for Get requests #766

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

mildwonkey
Copy link
Contributor

Description

Extends the optional configuration for API Domain Get methods:

  • parameters (alternative to supplying in the URL)
  • Headers
  • Timeout
  • Proxy configuration

I wanted to work through the PR review before I spend too much time on documentation so I opened separate docs tickets for each phase (I split up the main API Domain Optionality ticket by HTTP method to keep the PRs smallish).

Some notes of interest to reviews:

  • I added a validation & mutation step that reads in the APISpec and normalizes some values, such as the timeout string to a time.Duration, and stores the normalized values in unexported fields in the APISpec struct. I then made GetResources a method on the ApiDomain so I didn't have to pass the Spec around between methods.
  • Right now I'm treating ApiOptions in the Request as a full override of any top-level api options. I'm open to adding a flag to make that a merging behavior but wanted feedback before adding that (user or 🦄)

Related Issue

Fixes #765 (documentation is a separate PR)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@mildwonkey mildwonkey changed the title feat(api domain): Extend options for Get requests feat(api domain): extend options for Get requests Oct 28, 2024
@mildwonkey mildwonkey marked this pull request as ready for review October 28, 2024 14:28
@mildwonkey mildwonkey requested a review from a team as a code owner October 28, 2024 14:28
@mildwonkey mildwonkey mentioned this pull request Oct 29, 2024
6 tasks
src/pkg/domains/api/api.go Show resolved Hide resolved
src/pkg/domains/api/http_request.go Outdated Show resolved Hide resolved
src/pkg/domains/api/http_request.go Show resolved Hide resolved
src/test/e2e/api_validation_test.go Show resolved Hide resolved
Copy link
Collaborator

@meganwolf0 meganwolf0 left a comment

Choose a reason for hiding this comment

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

(This was originally a comment about how something didn't work as expected, but then I realized I had the wrong spec structure 🤦‍♀️ )
Anyway, did some local testing and everything worked swimmingly!

meganwolf0
meganwolf0 previously approved these changes Oct 31, 2024
@mildwonkey mildwonkey self-assigned this Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In Review
Development

Successfully merging this pull request may close these issues.

Extend Get options
3 participants