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 workflow #6308

Merged
merged 3 commits into from Nov 2, 2021
Merged

Add OpenAPI workflow #6308

merged 3 commits into from Nov 2, 2021

Conversation

nielsvanvelzen
Copy link
Member

@nielsvanvelzen nielsvanvelzen commented Jul 17, 2021

Changes
Add a new workflow called that does the following things:

  • Generate OpenAPI spec for master branch
    • although already available via azure on repo.jellyfin.org, this adds it the the GitHub interface
  • Generate OpenAPI spec for master branch
    • Useful when I need to test something with the Kotlin SDK, or to inspect changes in the file
  • Calculate difference between PR spec and baseline (target branch) spec using openapi-diff
    • Diff is rendered using markdown and added as a comment to the PR
    • If the PR is updated it will update the existing comment
    • Comment uses a <details> element to prevent skyscraper tall comments
      • For example, when updating BaseItemDto

The comments will help reviewers to determine if a PR contains breaking API changes, which might not be easy to notice in the code reviews.

To do
Some work left for this PR that I need some help with:

  • Need to use pull_request_target instead of pull_request BUT this would allow a malicious actor to expose secrets by making changes to the .NET code. No idea how to fix.
    • Need to use jellyfin-bot for comment
  • Detect the commit of the base branch the head branch is based on to use as a baseline to prevent changes made in the API in base showing up in the diff.
    • example: property removed from baseitemdto in master would also show up in this PR if the change wasn't in master when this branch was created.
    • I don't think this is a huge issue right now as API changes are scarce, it could become an issue with jf 11 dev though.

Issues

Example
See my personal fork for an example of this workflow: nielsvanvelzen#91

Issues

@nielsvanvelzen
Copy link
Member Author

I'm marking the PR as ready now since it does work, the "to do" list contains 2 items that should be considered before merging though.

@ferferga
Copy link
Member

The pull_request_target event checkouts code from the base branch instead (that's what I understand from docs).

However, for extra peace of mind, why not run this on master commits?

@nielsvanvelzen
Copy link
Member Author

However, for extra peace of mind, why not run this on master commits?

We need the openapi.json file for both the PR branch and the target (=master) branch to compare.

Copy link
Contributor

@h1dden-da3m0n h1dden-da3m0n left a comment

Choose a reason for hiding this comment

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

Looks good otherwise

.github/workflows/openapi.yml Show resolved Hide resolved
.github/workflows/openapi.yml Show resolved Hide resolved
.github/workflows/openapi.yml Outdated Show resolved Hide resolved
.github/workflows/openapi.yml Outdated Show resolved Hide resolved
.github/workflows/openapi.yml Show resolved Hide resolved
@cvium cvium merged commit eec4293 into jellyfin:master Nov 2, 2021
@nielsvanvelzen nielsvanvelzen deleted the openapi branch November 2, 2021 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants