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

chore: automate openapi schema list #6463

Merged
merged 6 commits into from
Mar 8, 2024

Conversation

gastonfournier
Copy link
Contributor

@gastonfournier gastonfournier commented Mar 7, 2024

About the changes

This PR automates the generation of exported open api schemas on pre-commit, removing some manual steps and also standardizing the process. The schema list definition now looks way simpler:

/*
* All schemas in `openapi/spec` should be listed here.
* Instead of listing them all maunally, exclude those that are not schemas (maybe they should be moved elsewhere)
*/
import * as importedSchemas from './spec';
const {
constraintSchemaBase,
unknownFeatureEvaluationResult,
playgroundStrategyEvaluation,
strategyEvaluationResults,
...exportedSchemas
} = importedSchemas;
export const schemas: UnleashSchemas = exportedSchemas;

Also added

/**
* Auto-generated file by update-openapi-spec-list.js. Do not edit.
* To run it manually execute `node update-openapi-spec-list.js` from .husky
*/
for devs

Copy link

vercel bot commented Mar 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Mar 8, 2024 1:52pm
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Mar 8, 2024 1:52pm

Comment on lines +43 to +46
constraintSchemaBase,
unknownFeatureEvaluationResult,
playgroundStrategyEvaluation,
strategyEvaluationResults,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are not schemas, but they are exported inside schema files. I don't think it's a problem but at least it posts a question on whether they should be there or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. I think I'm responsible for all of these, actually 😅 I think some of them make sense to keep where they are, but others maybe not. I don't think unknownFeatureEvaluationResult is actually used anywhere 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to fix them right now, and we can continue growing this list if needed, but at least it will pop the question. Personally, I moved the ones I introduced https://github.com/Unleash/unleash/pull/6463/files#r1516086818 but it would have been fine to add them to this exception list.

@@ -61,4 +61,4 @@ export const clientMetricsEnvSchema = {
},
} as const;

export type ClientMetricsSchema = FromSchema<typeof clientMetricsEnvSchema>;
export type ClientMetricsEnvSchema = FromSchema<typeof clientMetricsEnvSchema>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one was a bad copy paste that was spotted thanks to automating the exports

Comment on lines +5 to +6
export const TAG_MIN_LENGTH = 2;
export const TAG_MAX_LENGTH = 50;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two were moved instead of excluding them from the schemas

@@ -2,3 +2,10 @@
. "$(dirname -- "$0")/_/husky.sh"

npx lint-staged

node .husky/update-openapi-spec-list.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using node instead of bash, because it should be available in local dev

Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

This looks good to me! 😄 While I'm generally not a big fan of pre-commit hooks, I think automating this is great. Is there a way to do it that's less invasive?

Comment on lines +43 to +46
constraintSchemaBase,
unknownFeatureEvaluationResult,
playgroundStrategyEvaluation,
strategyEvaluationResults,
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. I think I'm responsible for all of these, actually 😅 I think some of them make sense to keep where they are, but others maybe not. I don't think unknownFeatureEvaluationResult is actually used anywhere 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this always write the file? How long does it take?

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'm generally not a big fan of pre-commit hooks

That makes 2 of us, but this is a quick way to find out whether this is a good solution or not. There are other alternatives, some less invasive, but this will help us understand what are our needs.

Does this always write the file?

Yes, only on commit, but it checks if the file was modified, and only then includes it in the change.

How long does it take?

Around 30ms

@gastonfournier gastonfournier merged commit da41d3d into main Mar 8, 2024
7 checks passed
@gastonfournier gastonfournier deleted the auto-generate-openapi-schema-list branch March 8, 2024 13:58
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