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 specification generation #178

Merged
merged 12 commits into from
Jan 20, 2025

Conversation

romalytvynenko
Copy link
Contributor

@romalytvynenko romalytvynenko commented Jan 9, 2025

Hey 🙌

This PR introduces automatic API specification generation using Scramble.

How it works

dedoc/scramble is required as development dependency. For spec generation testbench is used: ./vendor/bin/testbench scramble:export creates the specification file.

How CI part works

Using GitHub Actions, dedoc/scramble-pro is installed (provides support for packages the API is built with) and after that the specification can be generated (scramble:export).

Next, a very opinionated step happens. GHA clones docs repository into a subfolder. Then, the API specification file is being generated in that newly created docs repository folder (docs-repository/api-reference/openapi.json). After that, GHA commits and pushes changes (if any) to the docs repository. This can be improved though! I imagined something like pushing to some dedicated branch of docs repo and creating a PR from that branch (if not existing), so the maintainers can manually accept the updated specification.

This step is very opinionated and I was curious if I can make it work. If keeping the spec file within this repository is fine and it will somehow then be moved to docs repository, I will drop this part!

Notes

  1. Currently the core and CI depends on development branches of Scramble. This is due to me finding the generated API specification inaccuracies – I want to update Scramble to fix/improve such inconsistencies. After that is done, I tag versions an use them.
  2. I removed redundant annotations from controllers. However the documentation for API parameters is pretty valuable. And currently there is no attribute in Scramble that allows you to do that. So I am going to implement such attribute and use it. Let me know if removing existing annotations here is not okay, I will roll back the change.

Further improvements

  • Ensure PHPStan is happy
  • If a constant is referenced in allowedIncludes (or other methods; for example ->allowedIncludes(self::ALLOWED_INCLUDES)), the corresponding expected parameters are not documented
  • Add a way to document parameters manually with #[Parameter(...)] and #[QueryParameter(...)], ... etc attributes
  • Think an decide about how JSON:API should be documented (optional attributes, links, meta etc).
  • Remove Request suffix from the laravel data objects that are used only as a requests

The generated specification demo: https://elements-demo.stoplight.io/?spec=https://gist.githubusercontent.com/romalytvynenko/b0c750bc3f66177b488913d8bc1dcf34/raw/b4b28031d148b84a3ea873d5c624775f8b67ceae/spec3.json

Let me know what you think!

@jbrooksuk
Copy link
Member

@romalytvynenko this is so good, thank you!

We'll keep this in draft until Scramble has the new features merged, so we're not pinned to a specific branch.

@romalytvynenko
Copy link
Contributor Author

Just as a small update, working on that Request suffix issue 🙌

@romalytvynenko
Copy link
Contributor Author

@jbrooksuk just wanted to let you know that I've released alpha versions of Scramble and unpinned this implementation from branches!

While I will continue working on improving Scramble so it covers Cachet's API better, this branch can now be reviewed and merged 🙌

@romalytvynenko romalytvynenko marked this pull request as ready for review January 20, 2025 15:13
Copy link
Member

@jbrooksuk jbrooksuk left a comment

Choose a reason for hiding this comment

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

First, quick pass at a review.

Comment on lines 6 to 7
DOCS_REPOSITORY: romalytvynenko/cachet-docs
DOCS_BRANCH: feat/api-docs-generation
Copy link
Member

Choose a reason for hiding this comment

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

We need to update these to the real docs and main branch.

Comment on lines 44 to 45
git config --global user.name "GitHub Actions"
git config --global user.email "<>"
Copy link
Member

Choose a reason for hiding this comment

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

I think we can replace this with the setup-git-user workflow, https://github.com/marketplace/actions/setup-git-user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, could not make it work, but found this gem used in other Cachet's workflows! I'll use it

https://github.com/stefanzweifel/git-auto-commit-action

composer require dedoc/scramble-pro:0.7.0-alpha.1 --dev

- name: Checkout documentation repository
uses: actions/checkout@v2
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uses: actions/checkout@v2
uses: actions/checkout@v4

runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v2
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uses: actions/checkout@v2
uses: actions/checkout@v4

}

Scramble::afterOpenApiGenerated(function (OpenApi $openApi) {
$openApi->info->description = 'API documentation for Cachet, the open source status page system.';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$openApi->info->description = 'API documentation for Cachet, the open source status page system.';
$openApi->info->description = 'API documentation for Cachet, the open-source, self-hosted status page system.';

@romalytvynenko
Copy link
Contributor Author

Applied the requested changes!

@jbrooksuk
Copy link
Member

Let's go! :shipit:

@jbrooksuk jbrooksuk merged commit b8ea810 into cachethq:main Jan 20, 2025
25 checks passed
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.

2 participants