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

Api resource types #84

Merged
merged 4 commits into from
Jun 21, 2021
Merged

Api resource types #84

merged 4 commits into from
Jun 21, 2021

Conversation

engelke
Copy link
Contributor

@engelke engelke commented Jun 21, 2021

All currently defined resource types now fully implemented. API enhancements likely are needed following up.

@engelke engelke requested a review from grayside June 21, 2021 20:50
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 21, 2021
@engelke engelke requested a review from ace-n June 21, 2021 20:50
Copy link
Collaborator

@grayside grayside left a comment

Choose a reason for hiding this comment

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

Approved! I think we're close to the point where we need to backfill test coverage and start requiring it.

@@ -0,0 +1,9 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Was this intended to be committed? If so perhaps it was meant for a test or docs directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a file explaining how to exercise the API with curl commands. These two json files are sample data supporting those curl commands.

@@ -0,0 +1,3 @@
{
"goal": 2000.00
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: was this meant to be committed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as a above.

@grayside grayside added component: content-api Related to the Content API. needs decision record This issue or change is significant and we should create a decision record. labels Jun 21, 2021
@grayside
Copy link
Collaborator

I approved but we've also slid in a number of decisions like REST and PATCH method without decision records. Flagging that as necessary but I think the AI is to identify the decisions and file those as new issues.

@engelke
Copy link
Contributor Author

engelke commented Jun 21, 2021

Test coverage is needed, as are documentation of decisions. One such decision is the behavior of PATCH and DELETE relative to etags.

@engelke engelke merged commit 192e863 into main Jun 21, 2021
@engelke engelke deleted the api-resource-types branch June 21, 2021 21:19
grayside pushed a commit that referenced this pull request Jul 22, 2021
* Added most of campaigns resource

* All operations for campaigns and approvers

* Causes and Donors added

* Add resource types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. component: content-api Related to the Content API. needs decision record This issue or change is significant and we should create a decision record.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants