search

Azure / azure-sdk-tools

Tools repository leveraged by the Azure SDK team.
MIT License
114 stars 180 forks source link

[LintDiff] Pin version of @microsoft.azure/openapi-validator-rulesets #7613

Open mikeharder opened 9 months ago

mikeharder commented 9 months ago

LintDiff pins the version of @microsoft.azure/openapi-validator, but it does not pin the version of @microsoft.azure/openapi-validator-rulesets. @microsoft.azure/openapi-validator does not pin either. The end result is LintDiff is using @microsoft.azure/openapi-validator-rulesets@latest, which can break any time a new version is published.

Instead, LintDiff should somehow pin the version of @microsoft.azure/openapi-validator-rulesets, so it is only updated via a PR merged to LintDiff. LintDiff could pin the version directly, or @microsoft.azure/openapi-validator could pin instead of float.

weshaggard commented 9 months ago

I thought we weren't pinning by design because we wanted to make releasing ruleset updates by just release a new package. Of course, if we do use that strategy then we need to be extra careful about releasing this ruleset package. Either way I think we need to figure out our testing strategy for these packages. I think it would be awesome if we could get to a place where we run these package updates against all the specs in the repo. That might be pretty noisy right now but perhaps we can create a baseline and just make sure that we aren't introducing any new issues.

konrad-jamrozik commented 9 months ago

@mikeharder @weshaggard yes, it is not pinned on purpose. We have mechanisms for such tests: the stagingOnly: https://github.com/Azure/azure-openapi-validator/blob/main/CONTRIBUTING.md#how-to-set-a-spectral-rule-to-run-only-in-staging

Because it is not pinned, it is easy to deploy something to staging. Once the rule is verified in staging, it can be deployed to prod by bumping the ruleset package ver. And again, this is on purpose not pinned. If it would be pinned, then releasing new ruleset would require updates to openapi-alps which we do not want. See this doc:

IMPORTANT: Changes to the AutoRest extension package, used by Production LintDiff, require additional code updates to openapi-alps ADO repository, and deployment of them. Work with this tool owner to apply these steps. Example of such past deployment is given https://github.com/Azure/azure-sdk-tools/issues/6071#issuecomment-1530128107, with this PR updating the LINT_VERSION value.

konrad-jamrozik commented 9 months ago

We are having this conversation because of this:

However, the stagingOnly system is not really well suited for this scenario. It is designed to update one rule art a time, but the problem was caused by schema change to all rulesets:

I provide more details in https://github.com/Azure/azure-openapi-validator/issues/653

mikeharder commented 9 months ago

I think we can change how these packages are versioned and consumed, that will satisfy all our requirements but also follow the best practices of the JS ecosystem.

Packages should use proper semver for their own version (breaking changes increment major, additive changes increment minor, bug fixes increment patch). Packages should typically depend on other packages using ^, which floats to the latest compatible version. There may be exceptions if you own both packages, where you choose to pin to guarantee package versions are aligned (which may be appropriate for the openapi-validator-rulesets package family).

Applications (like Swagger LintDiff) should have a package.json expressing their dependencies (pinned or floated), and a package-lock.json that pins the version of all direct and transitive dependencies. Thus, application dependencies should never change at runtime. Only via a PR that updates package-lock.json.

I understand Swagger LintDiff does not currently work this way, but we should start moving in this direction.

Snippet added by @konrad-jamrozik based on our conversation:

mikeharder: The bottom line is the versions of all openapi-validator* packages should be pinned in some file in the repo package-lock.json or a custom config file with variables that get passed to autorest.

konrad-jamrozik commented 9 months ago

@mikeharder makes sense to me. Currently the way it works, on a high level, is that as soon a PR is merged, CI triggers and publishes new beta package without any approvals or what not. Then there is a separate CI check, "Swagger LintDiff (Staging)" (forgot exact title), that runs based off the beta packages. Unfortunately, the source code for it was done by copy-paste-adapt, so the code behind this staging CI check is almost identical to the prod LintDiff, but naturally has a different AutoRest CLI invocation that relies on latest beta packages.

konrad-jamrozik commented 9 months ago

I am going to make some improvements to the CI process. Tracking it here: