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

Add OpenAPI documentation for request endpoints #10637

Merged
merged 8 commits into from
Jan 21, 2021

Conversation

krauselukas
Copy link
Contributor

Replace this line with a description of your changes. Please follow the suggestions in the comment below.


If this PR requires any particular action or consideration before deployment,
please check the reasons or add your own to the list:

  • Requires a database migration that can cause downtime. Postpone deployment until maintenance window[1].
  • Contains a data migration that can cause temporary inconsistency, so should be run at a specific point of time.
  • Changes some configuration files (e.g. options.yml), so the changes have to be applied manually in the reference server.
  • A new Feature Toggle[2] should be enabled in the reference server.
  • Proper documentation or announcement has to be published upfront since the introduced changes can confuse the users.

[1] https://github.com/openSUSE/open-build-service/wiki/Deployment-of-build.opensuse.org#when-there-are-migrations
[2] https://github.com/openSUSE/open-build-service/wiki/Feature-Toggles-%28Flipper%29#you-want-real-people-to-test-your-feature

@krauselukas krauselukas added Documentation 📖 Things regarding our documentation review-app Apply this label if you want a review app started labels Jan 15, 2021
@obs-bot
Copy link
Collaborator

obs-bot commented Jan 15, 2021

Review app will appear here: http://obs-reviewlab.opensuse.org/krauselukas-api_docsrequest

@krauselukas
Copy link
Contributor Author

Request

@krauselukas krauselukas force-pushed the api_docs/request branch 3 times, most recently from 68ee114 to 9c70faf Compare January 20, 2021 12:55
@krauselukas krauselukas marked this pull request as ready for review January 20, 2021 12:57
@dmarcoux
Copy link
Contributor

We should add a note to the endpoints which are running expensive operations. The endpoint POST /request/{id}?cmd=diff is for sure one of them. I believe GET /request?view=collection could also be marked as such.

@@ -0,0 +1,63 @@
post:
summary: Get the diff for all by the request affected packages
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this sentence. Is this Get the diff for all packages affected in the request? I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, the sentence is a bit off, will change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied your suggestion :)

@krauselukas
Copy link
Contributor Author

We should add a note to the endpoints which are running expensive operations. The endpoint POST /request/{id}?cmd=diff is for sure one of them. I believe GET /request?view=collection could also be marked as such.

Yes, might be a good idea to mention it, since its also mentioned in the general description of the API

@hennevogel
Copy link
Member

Yes, might be a good idea to mention it, since its also mentioned in the general description of the API

No please. Calling an inexpensive operation 300 times concurrently is wrong the same way. Let's keep this simple.

@krauselukas
Copy link
Contributor Author

Yes, might be a good idea to mention it, since its also mentioned in the general description of the API

No please. Calling an inexpensive operation 300 times concurrently is wrong the same way. Let's keep this simple.

True, haven't seen it from this perspective, lets keep it simple then 👍

Copy link
Member

@eduardoj eduardoj left a comment

Choose a reason for hiding this comment

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

Huge work! Thanks!

src/api/public/apidocs-new/paths/request.yaml Show resolved Hide resolved
src/api/public/apidocs-new/paths/request_request_id.yaml Outdated Show resolved Hide resolved
src/api/public/apidocs-new/paths/request_request_id.yaml Outdated Show resolved Hide resolved
src/api/public/apidocs-new/paths/request_request_id.yaml Outdated Show resolved Hide resolved
src/api/public/apidocs-new/paths/request.yaml Outdated Show resolved Hide resolved
src/api/public/apidocs-new/paths/request.yaml Outdated Show resolved Hide resolved
@krauselukas
Copy link
Contributor Author

@eduardoj I addressed your review, please have another look :)

Copy link
Member

@eduardoj eduardoj left a comment

Choose a reason for hiding this comment

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

@eduardoj I addressed your review, please have another look :)

Looks good to me. Thanks a lot!

@krauselukas krauselukas merged commit b7fb165 into openSUSE:master Jan 21, 2021
@dmarcoux dmarcoux added the API Things regarding our API label Jan 26, 2021
@krauselukas krauselukas deleted the api_docs/request branch March 4, 2022 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Things regarding our API Documentation 📖 Things regarding our documentation review-app Apply this label if you want a review app started
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants