eng, remove api-version required check, and infer sdk release type from api-version if not provided#48706
eng, remove api-version required check, and infer sdk release type from api-version if not provided#48706XiaofeiCao wants to merge 5 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the TypeSpec management-plane SDK automation script to infer whether to generate a beta vs stable release when sdkReleaseType isn’t provided, based on generated metadata.
Changes:
- Added
infer_sdk_release_type(...)to derivesdkReleaseTypefrom generated*_metadata.json. - Adjusted TypeSpec project generation flow to default to beta, then (optionally) infer and apply stable/beta selection for versioning after generation.
- Removed the previous self-serve parameter validation gate around
apiVersion/sdkReleaseType.
| run_mode: str = config["runMode"] if "runMode" in config else None | ||
|
|
||
| if run_mode == "release" or run_mode == "local": | ||
| verify_self_serve_parameters(api_version, sdk_release_type) |
There was a problem hiding this comment.
Do we still call verify_self_serve_parameters somewhere? This validation still needs to happen when users specify 'api-version' and 'sdk-release-type' in the release and local scenarios. That said, the validation can be removed only when they are not defined.
There was a problem hiding this comment.
Do we still call verify_self_serve_parameters somewhere?
After this PR, we won't.
The tricky part is "what" to validate. when they are not defined is vague.
E.g.
- Should we still throw exception when
api-versionis specified GA andsdk_release_typeis specified beta? - If so, should we also throw exception when
api-versionis not specified and inferred GA, whilesdk_release_typeis specified beta? - Also, when
api-versionis specified andsdk_release_typeis not, should we throw exception when spec is multi-service(e.g. Compute)?
When we make api-version optional, we are already hoping user to know what they are doing. I'd say validation makes little sense now.
There was a problem hiding this comment.
I'm ok for removing this validation given this automation won't be invoked directly by users. The most important scenario to defense is with using preview api-version and GA sdk-release-type. This check is already in place of pipeline.
| # Infer sdk release type from generated metadata when not explicitly provided | ||
| if not sdk_release_type and sdk_folder and module: | ||
| inferred_type = infer_sdk_release_type(sdk_root, sdk_folder, module) | ||
| release_beta_sdk = inferred_type == "beta" |
There was a problem hiding this comment.
We may need to have an update to pom.xml, to remove the revapi.skip line, if target version is stable.
Previously it is done in emitter, based on version_client. And this likely would not be correct as #48706 (comment)
There was a problem hiding this comment.
Thanks, updated. Also removed release_beta_sdk parameter from generate_from_typespec function.
Test PRs:
- inferred stable: Test keyvault XiaofeiCao/azure-sdk-for-java#25
- inferred beta: Test servicenetworking XiaofeiCao/azure-sdk-for-java#26
There was a problem hiding this comment.
test on keyvault is meaningless ? I thought we don't update pom on premium (could be wrong though)
There was a problem hiding this comment.
Yeah, created new PR for inferred stable:
Also added unit test for revapi.skip.
(Not related)Strange, seems there's no increment-version PR since last storagemover release.. Maybe we need to make one:
https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/storagemover/azure-resourcemanager-storagemover/CHANGELOG.md
weidongxu-microsoft
left a comment
There was a problem hiding this comment.
We may need to have a post generate update to `revapi.skip
eng/automation/generate.py
Outdated
| @@ -321,6 +335,11 @@ def sdk_automation_typespec_project(tsp_project: str, config: dict) -> dict: | |||
There was a problem hiding this comment.
Note here that it uses a potentially wrong generate_beta_sdk value. It likely affect version_client.txt
So you may need to update it again, after success (and update the CHANGELOG too?)
There was a problem hiding this comment.
Currently we have that after generation:
azure-sdk-for-java/eng/automation/generate.py
Lines 323 to 333 in 7387969
For changelog:
azure-sdk-for-java/eng/automation/generate.py
Line 359 in 7387969
The generate_beta_sdk parameter is used for passing package-version to emitter, possibly for setting revapi.skip.
My local test also seems fine. If we can't rely on release type in emitter, guess we could consider removing it, after this PR.
There was a problem hiding this comment.
We seem do have another pair here (I think this path is only for mgmt lib)?
azure-sdk-for-java/eng/automation/generate_utils.py
Lines 611 to 618 in 7387969
(so we seem updated version_client twice, in the script, on mgmt lib?)
There was a problem hiding this comment.
Yes, here is for getting the correct current_version for setting into package-version for emitter. Previous logic to put it here, is due to module parse logic also being here.
Guess now we can remove line 613-616, since generate_beta_sdk cannot be trusted any more.
When SDK release type is beta (either explicitly passed or inferred from api-version), ensure <revapi.skip>true</revapi.skip> is present in the generated pom.xml: add it if missing, or change false to true. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The preview version is set again after generation with the correct release_beta_sdk value, so the pre-generation call defaults to True. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| # Infer sdk release type from generated metadata when not explicitly provided | ||
| if not sdk_release_type and sdk_folder and module: | ||
| inferred_type = infer_sdk_release_type(sdk_root, sdk_folder, module) | ||
| release_beta_sdk = inferred_type == "beta" |
There was a problem hiding this comment.
test on keyvault is meaningless ? I thought we don't update pom on premium (could be wrong though)
eng/automation/generate.py
Outdated
|
|
||
| # Ensure revapi.skip=true for beta releases | ||
| if release_beta_sdk: | ||
| ensure_revapi_skip(os.path.join(sdk_root, output_folder, "pom.xml")) |
There was a problem hiding this comment.
We either need this to handle both way (ensure_revapi_not_skip as well), or update emitter to remove the logic
https://github.com/microsoft/typespec/blob/main/packages/http-client-java/generator/http-client-generator-mgmt/src/main/java/com/microsoft/typespec/http/client/generator/mgmt/template/FluentPomTemplate.java#L34-L36
As in most cases, the current version of version_client is a beta, so emitter will add revapi.skip.
Therefore, most cases, it would be ensure_revapi_not_skip upon stable.
We can remove the emitter logic, but it seems still valid when user run tsp-client update in repo, where the current version in version_client IS valid and IS the final version.
Hence I guess we just make a util method to always add/update revapi.skip according to beta/stable?
Renamed ensure_revapi_skip to update_revapi_skip. Now sets revapi.skip=true for beta releases and revapi.skip=false for stable releases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Tested locally
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines