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

[FEATURE REQ] New readme.md repo PR check to enforce Azure service versioning #31506

Open
JeffreyRichter opened this issue Nov 11, 2024 · 4 comments
Assignees
Labels
feature-request This issue requires a new behavior in the product in order be resolved.

Comments

@JeffreyRichter
Copy link
Member

API Spec link

NA

API Spec version

NA

Please describe the feature.

Each Azure service version must consist of operations that all share the same api-version.
Also, a service cannot mix & match stable and preview operations.
This is required for docs, SDK versions, and more.

This feature request is to add a PR check that looks at a new readme.md file, finds the latest default "tag".
This value MUST be of the format YYYY-MM-DD and can have an optional -preview suffix, or the tag value is an error and the PR cannot be merged.

In the readme.md's "input-file:" section, all paths MUST have a version that matches the tag value exactly, or the PR cannot be merged. SIDENOTE: This ensures that a preview version consists of only preview json files and a stable version consists of only stable json files.

It is an error and the PR cannot be merged if a json file exists in the version folder that is missing from the "input-file:" section. This catches a bug where the json file exists but was forgotten to be added to the "input-file:" section.

@JeffreyRichter JeffreyRichter added the feature-request This issue requires a new behavior in the product in order be resolved. label Nov 11, 2024
@mikeharder
Copy link
Member

@JeffreyRichter: This has significant overlap with #29065. I'm fine merging the issues or tracking separately.

Your request would be implemented in a new check named something like Versioning Validation, consisting of a TypeScript-based tool encapsulating the business logic, and a GitHub Action to run the check on PRs.

What should be done about existing specs? Try to validate them and fix them, either in bulk or by adding suppressions? Or make this check only run on PRs, so specs are only forced to fix (or suppress) in their next PR?

Note this may come as a surprise, when spec authors make a small bugfix PR, and are confronted with a versioning violation. If needed, we can try to reduce the scope of when the check runs. It's fairly easy to only run when certain files (like readme.md are edited). It's more difficult to scope to changes within a file (like "only run if a new version was added to readme.md").

Or, we can just tell spec authors to add a suppression in suppressions.yaml in their spec folder. But, if we want spec authors to fix this eventually, we'll need to ensure the suppressions have a limited scope, say limited to a single spec version.

@JeffreyRichter
Copy link
Member Author

You can merge #29065 and this issue. I had forgotten that #29065 was created. I added something to this issue missing from #29065 (an error to have a file in the version dir not in the input-file section) and #29065 has something that I'm missing from this issue (stable and preview API versions ❗cannot❗ have the same date) but we do want it ALL.

Right now, only run on PRs that have changed the readme.md file. Let's hold off on allowing suppressions. But, maybe Mike or I could add a label to counter the "BrokenServiceVersioning" (my initial proposed label name - may change) such as "BrokenServiceVersioning-TemporaryException" (also initial proposed label name - may change).

It would be nice to have a tool that can identify existing broken specs and then @mikekistler and & I can reach out to those teams and work with them to get things fixed. You won't be able to auto-fix existing specs that are broken; the service team must make some decisions first.

@mikeharder
Copy link
Member

mikeharder commented Nov 12, 2024

Right now, only run on PRs that have changed the readme.md file

I don't think this is sufficient, and we'll need to run on any changes under the folder containing readme.md. This is required to catch issues like "added a json file in the version folder that's not listed in readme.md".

Let's hold off on allowing suppressions.

Easy to add later.

But, maybe Mike or I could add a label to counter the "BrokenServiceVersioning" (my initial proposed label name - may change) such as "BrokenServiceVersioning-TemporaryException" (also initial proposed label name - may change).

Since the problem is in the source code (readme.md), rather than specific to the individual PR, I think suppressions should also live in the source code. So instead of a PR label, I'd recommend adding an entry to the spec's suppressions.yaml. We can even require this suppression to be limited to a single version if we want, so the spec author will be required to fix the problem in their next spec version.

It would be nice to have a tool that can identify existing broken specs and then @mikekistler and & I can reach out to those teams and work with them to get things fixed. You won't be able to auto-fix existing specs that are broken; the service team must make some decisions first.

This should be standard for all new checks, which should support all the following "modes":

  1. Local dev machine, verify one spec
  2. Local dev machine, verify all specs
  3. PR, verify impacted specs and block merge
  4. CI, verify all specs (with suppressions unique to the "all" check), requires immediate attention if failing

@mikeharder
Copy link
Member

We may also want to require a "formal azure service name" in the readme.md, that will appear in the documentation at the Azure Learn website.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request This issue requires a new behavior in the product in order be resolved.
Projects
None yet
Development

No branches or pull requests

3 participants