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

Pull Requests: Create swagger spec #8168

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Conversation

N-o-Z
Copy link
Member

@N-o-Z N-o-Z commented Sep 17, 2024

Closes #8167

Change Description

Create swagger spec, generated code and controller stubs for pull requests

Background

Epic

@N-o-Z N-o-Z added exclude-changelog PR description should not be included in next release changelog pull-requests labels Sep 17, 2024
@N-o-Z N-o-Z requested review from itaigilo and a team September 17, 2024 01:27
@N-o-Z N-o-Z self-assigned this Sep 17, 2024
Copy link

github-actions bot commented Sep 17, 2024

♻️ PR Preview 48679e6 has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

Copy link

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed

Copy link

E2E Test Results - Quickstart

10 passed

@N-o-Z N-o-Z force-pushed the task/pulls-create-swagger-8167 branch from bf02d4d to 61c150c Compare September 17, 2024 01:43
@N-o-Z N-o-Z force-pushed the task/pulls-create-swagger-8167 branch from 61c150c to 6792b70 Compare September 17, 2024 01:49
This reverts commit 6792b70.
@N-o-Z
Copy link
Member Author

N-o-Z commented Sep 17, 2024

Actual changed files:

  • swagger.yaml
  • controller

All the rest are auto generated code

Copy link
Contributor

@itaigilo itaigilo left a comment

Choose a reason for hiding this comment

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

LGTM,
See minor comments.

It'll probably require some changes soon once I'll start implementing the UI, but we'll work on this as we go.

writeResponse(w, r, http.StatusNotImplemented, nil)
}

func (c *Controller) DeletePullRequest(w http.ResponseWriter, r *http.Request, repository string, pullRequest string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be repositoryID and pullRequestID.
Also relevant for GetPullRequest and UpdatePullRequest.

Copy link
Member Author

Choose a reason for hiding this comment

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

For repository our convention in all other functions is repository
I'll update pullRequest

- in: path
name: repository
required: true
schema:
Copy link
Contributor

@itaigilo itaigilo Sep 17, 2024

Choose a reason for hiding this comment

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

What about description?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty straightforward, I don't think repository needs any elaboration

description: too many requests
default:
$ref: "#/components/responses/ServerError"
patch:
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that in the Design Issue it's designed different -
But what's written in the PR is far better (hence I think the design should be updated).

@N-o-Z N-o-Z merged commit 066d8f5 into master Sep 17, 2024
38 of 39 checks passed
@N-o-Z N-o-Z deleted the task/pulls-create-swagger-8167 branch September 17, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude-changelog PR description should not be included in next release changelog pull-requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pull Requests MVP: Create swagger spec
2 participants