Design: Automated Headless api-docs upload #56
Conversation
| The platform that manages the OpenAPI Documentation static website, [Redoc](https://github.com/Redocly/redoc), only interacts with `.json` files. So we need to convert our `src/api-docs.js` file into a `.json` one whenever we want to use this platform. | ||
|
|
||
| A script should be built to make this conversion on the `scripts` folder, outputting the results to the `tmp/` folder. All following steps will be executed on this converted file. Some specific conditions to execute: | ||
| - Remove the `securitySchemes` from the `components` property |
There was a problem hiding this comment.
Why do we need to remove it? And what are they used for since we will remove them? Shouldn't we remove them from the src/api-docs.js file?
There was a problem hiding this comment.
The current contents of this property are:
"securitySchemes": {
"ApiKeyAuth": {
"type": "apiKey",
"in": "header",
"name": "X-API-KEY"
}
}So, it's not that we're sharing any sensitive information.
@pedroferreira1 , since you have more experience with this workflow, could you share with us why we currently remove this property before publishing the api-docs?
There was a problem hiding this comment.
I feel we can remove them. We added it at first because we allow the headless wallet to have an optional configuration for API keys. However, the users got confused because, in the documentation, ALL endpoints were described with API keys, so they thought it was required. Since then, I decided to remove this part every time I generated a new docs page.
I think we can remove it as @luislhl suggested.
There was a problem hiding this comment.
Removal suggested on a336ca7
| - A lint should be executed on the generated docs, displaying its analysis results | ||
| - A local server should be fired, allowing the developer to visualize the results of the doc changes | ||
|
|
||
| This would be an automated version of manually executing the following commands: |
There was a problem hiding this comment.
Maybe we should make sure to remove the tmp/api-docs.json file at the end of the process.
There was a problem hiding this comment.
I see no negative consequence of having this file lingering at the tmp folder:
- On the local dev environment it could be reused by the
preview-docscommand - On the CI environment, it would be destroyed as soon as the workflow is over
However I think it's a good practice to remove the tmp files if we have no explicit reason to keep it. 👍
Changed it on 9d4de6e
| > *Note:* The linter throws some errors on our current documentation. The first implementation of this script will certainly require a refactoring PR. | ||
|
|
||
| ### PR Validation | ||
| A GitHub Workflow, configured to run on every PR merging to `dev`, should run the conversion script and linter to check for errors on the generated documentation. This should be part of the CI process. |
There was a problem hiding this comment.
Why only on PRs merging to dev? What if someone creates stacked PRs?
There was a problem hiding this comment.
Our current default is to only run our workflows on PRs merging to dev ( or, in cases where this branch doesn't exist, on master / main ).
If it becomes really necessary to run this on a separate branch, it would always be possible to add the branch name on the workflow yml file on that specific branch / PR.
What do you think?
There was a problem hiding this comment.
I think the problem with running tests/linters only for PRs based on dev/master is that PRs based on another feature branch wouldn't get their tests run.
This could lead us to transfering bugs from one PR to another when merging them, or at least taking longer to give some feedback for the developer about problems in their code.
For instance, I got one from our board to show you this is not so rare to happen: HathorNetwork/hathor-core#645
This PR only had tests run on it because hathor-core is currently configured to run on any PR regardless of the base branch: https://github.com/HathorNetwork/hathor-core/blob/master/.github/workflows/main.yml#L10
There was a problem hiding this comment.
While I think this would be less necessary for a documentation feature, I think the overhead is very small ( the validation proposed is very fast and light on resources ) and the benefit is clear on the CI.
Removed the branch restriction on a6d072b
This feature automates the process of uploading the updated API Docs for the Headless Wallet, while giving the developer a preview of the contents, error checking and linting.
View rendered