-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
support track2 codegen in swagger pipeline #16257
support track2 codegen in swagger pipeline #16257
Conversation
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment: |
/check-enforcer override |
Make sure it's tested in env-test first. |
@PhoenixHe-msft yes, it's already tested in env-test and runs successfully |
/check-enforcer override |
@@ -0,0 +1,2 @@ | |||
#!/usr/bin/env bash | |||
track2-codegen-automation-for-pipeline --inputJsonPath=$1 --outputJsonPath=$2 --use=@autorest/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dw511214992 I realize this is a very old PR but after looking through it I have a few questions and suggestions.
- Where do we publish track2-codegen-automation-for-pipeline? It should probably be scripts in this repo
- Can we please move these scripts under eng/scripts?
- Can we please move the swagger_to_sdk_config.json under eng directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @weshaggard
- Answer for question 1: the codes of
track2-codegen-automation-for-pipeline
is in DevOps Repository now, and we planned to move it to js sdk repository in next month (I don't have time to do it in this month, and I'll do it once I have time. Sorry for it) - Answer for question 2: I agree to put
automation_generate.sh
in eng/scripts, and I'll do it. However, the tooltrack2-codegen-automation-for-pipeline
not only contains script for swagger pipeline, but also contains other tools, such as generating changelog, bumping version automatically, generate codes automatically in local. So I think it's better to put it incommon/tools
. - Answer for question 3: I don't think we can move
swagger_to_sdk_config.json
to other places because the swagger pipeline asks this file should be in the root of js sdk repository.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dw511214992
- Cool yes I would like to get that tool moved into the repo and if possible under the eng folder. I'm trying to get more of the common/tools folder under eng as well.
- It is perfectly fine for all the script automation to be in the JS repo we have lots of similar scripts already. Part of the reason we want it in the repo is so we can share stuff where possible.
- You can move it to another location but it requires a change in in the specs repo. See https://github.com/Azure/azure-rest-api-specs/blob/main/specificationRepositoryConfiguration.json#L35 as an example. Ideally we would also not call the config "track2" either, I know a number of others already are so I won't worry much but I generally try to avoid code names like track 2 and instead use the versioning of the tool instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weshaggard thanks, and I'll do these tasks together. About moving the scripts to eng folder, the code in DevOps Repository is complex (it's a project) and I'm not sure whether moving it to eng folder is a good solution. Anywhere, I'll rethink about it.
@sarangan12, Do we know if these changes will affect the data plane libraries in the swagger pipeline too? |
Looking at the code changes, I do not see anything specific to management libraries. It looks like it is common to both management and data plane libraries |
@sarangan12, Please confirm if that is desired. We have a few Track 1 data plane packages. We should ensure that these don't get overridden with Track 2 code unintentionally |
#11574 is related I believe |
Could you please explain why this change has been done? Per @ramya-rao-a conversation above, We have few Track 1 data plane packages. We do not want to want to override then with Track 2 code. With your changes, I do not see that the changes are specific to management plane. So, could you please confirm if your changes are specific to management plane? If not, we do not want these changes (and must be reverted) as they will affect the data plane packages. |
If we need different generation settings then we can split the config files into different paths similar to what I suggested in my earlier comment. |
@ramya-rao-a @sarangan12 @weshaggard I don't think it affect the data plane packages. The pipeline will be only triggered if the readme.md in swagger is configured to use this pipeline. For example (You can find commit here): |
@dw511214992 I'm not sure that is true. Looking at the configuration that drives these at https://github.com/Azure/azure-rest-api-specs/blob/main/specificationRepositoryConfiguration.json#L19 there are 2 JS configurations one is for the track1 mgmt which is pointed at the feature/v4 branch and the other is the track2 mgmt which points to the main repo. So unless one of those configurations also handles the track1 data plane then I think those might be broken currently. |
This PR is used to integrate track2 js mgmt codegen to swagger pipeline. After it's merged, I will create a PR to make changes in swagger repository.
When codegen is in peview, we specify the verison of codegen.
When codegen is GA, we will always use the latest codegen.