Skip to content

track2 @azure/arm-resources#16170

Merged
qiaozha merged 15 commits intomainfrom
dw/track2-arm-resource
Jul 15, 2021
Merged

track2 @azure/arm-resources#16170
qiaozha merged 15 commits intomainfrom
dw/track2-arm-resource

Conversation

@dw511214992
Copy link
Copy Markdown
Member

@dw511214992 dw511214992 commented Jul 2, 2021

No description provided.

@check-enforcer
Copy link
Copy Markdown

check-enforcer bot commented Jul 2, 2021

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:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

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:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

@weshaggard
Copy link
Copy Markdown
Member

weshaggard commented Jul 2, 2021

/azp run prepare-pipelines

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jul 2, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@weshaggard
Copy link
Copy Markdown
Member

weshaggard commented Jul 2, 2021

triggered prepare-pipelines to try and generate the pipeline for your new ci.yml

@weshaggard
Copy link
Copy Markdown
Member

weshaggard commented Jul 2, 2021

/azp run js - resources - ci

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jul 2, 2021

Azure Pipelines successfully started running 1 pipeline(s).

deyaaeldeen
deyaaeldeen reviewed Jul 2, 2021
View reviewed changes
deyaaeldeen
deyaaeldeen reviewed Jul 2, 2021
View reviewed changes
deyaaeldeen
deyaaeldeen reviewed Jul 2, 2021
View reviewed changes
sdk/resources/arm-resources/api-extractor.json
"dtsRollup": {
"enabled": true,
"untrimmedFilePath": "",
"publicTrimmedFilePath": "./esm/index.d.ts"
Copy link
Copy Markdown
Member

@deyaaeldeen deyaaeldeen Jul 2, 2021

Choose a reason for hiding this comment

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

@sarangan12 also please implement this change as well to follow what Track 2 data-plane packages do.

Suggested change
"publicTrimmedFilePath": "./esm/index.d.ts"
"publicTrimmedFilePath": "./types/arm-resources.d.ts"

Copy link
Copy Markdown
Contributor

@sarangan12 sarangan12 Jul 9, 2021

Choose a reason for hiding this comment

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

@deyaaeldeen Could you please explain what is the impact of not changing it for this release? (We can change it for the next one). Will there be any adverse customer impact or breaking change?

Copy link
Copy Markdown
Member

@deyaaeldeen deyaaeldeen Jul 9, 2021

Choose a reason for hiding this comment

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

No correctness issues here, my suggestion is purely to be consistent with data-plane packages.

deyaaeldeen
deyaaeldeen reviewed Jul 2, 2021
View reviewed changes
deyaaeldeen
deyaaeldeen reviewed Jul 2, 2021
View reviewed changes
sdk/resources/arm-resources/package.json
"types": "./esm/resourceManagementClient.d.ts",
"main": "./dist/index.js",
"module": "./esm/index.js",
"types": "./esm/index.d.ts",
Copy link
Copy Markdown
Member

@deyaaeldeen deyaaeldeen Jul 2, 2021

Choose a reason for hiding this comment

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

Suggested change
"types": "./esm/index.d.ts",
"types": "./types/arm-resources.d.ts",

Copy link
Copy Markdown
Contributor

@sarangan12 sarangan12 Jul 9, 2021

Choose a reason for hiding this comment

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

@deyaaeldeen Same here

Copy link
Copy Markdown
Member

@deyaaeldeen deyaaeldeen Jul 9, 2021

Choose a reason for hiding this comment

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

No correctness issues here, my suggestion is purely to be consistent with data-plane packages.

deyaaeldeen
deyaaeldeen reviewed Jul 2, 2021
View reviewed changes
sdk/resources/arm-resources/package.json Outdated
"uglify-js": "^3.4.9"
},
"homepage": "https://github.com/Azure/azure-sdk-for-js/tree/main/sdk/resources/arm-resources",
"homepage": "https://github.com/Azure/azure-sdk-for-js",
Copy link
Copy Markdown
Member

@deyaaeldeen deyaaeldeen Jul 2, 2021

Choose a reason for hiding this comment

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

this is weird. cc @sarangan12

Copy link
Copy Markdown
Contributor

@sarangan12 sarangan12 Jul 9, 2021

Choose a reason for hiding this comment

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

@deyaaeldeen This is a minor manual change, which is probably required only for the first time. I would not recommend doing any code level changes for this.

deyaaeldeen
deyaaeldeen reviewed Jul 2, 2021
View reviewed changes
deyaaeldeen
deyaaeldeen reviewed Jul 2, 2021
View reviewed changes
sarangan12
sarangan12 reviewed Jul 2, 2021
View reviewed changes
deyaaeldeen
deyaaeldeen reviewed Jul 2, 2021
View reviewed changes
@dw511214992 dw511214992 mentioned this pull request Jul 5, 2021
Closed
@sarangan12
Copy link
Copy Markdown
Contributor

sarangan12 commented Jul 9, 2021

@dw511214992 / @deyaaeldeen

I have gone through the comments. Among the comments, there are 3 things:

  1. Issue regarding URL Samples - A separate issue - Readme for mgmt pkgs should point to mgmt samples repo autorest.typescript#1009 has been created. I will take this issue for the 07/20 release.
  2. Issue regarding LICENSE - Will be complete with PR Change Const Enum to Enum autorest.typescript#1097
  3. Issue regarding homepage - This can be changed manually.

Now, for the rest of the comments, they seem to be valid. But, at this point, it needs further investigation (things such as publicTrimmedPath, outDir, etc). So, my suggestion is, if you feel that those items must be modified, Please do it manually for this PR and create one consolidated issue with autorest.typescript repository with detailed list of changes required. I can pick it up for the 07/20 release.

Please let me know if it sounds good.

@dw511214992
Copy link
Copy Markdown
Member Author

dw511214992 commented Jul 9, 2021 •
edited
Loading

@sarangan12 for the rest comments, I don't want to change them manually and include them in the first release. And Aftert codegen is released in 07/20, we can do the second release and include the rest comments.
about samples url and homepage url, it's better codegen can change it in this release？ If you don't have enough time to do it, we will change them manualy.
thanks

@sarangan12
Copy link
Copy Markdown
Contributor

sarangan12 commented Jul 9, 2021

@dw511214992 We have already created the issue for samples URL. The homepage url might be tricky. But, I can try doing that programmatically. So, include that also when you are creating the issue for the rest of the comments.

@deyaaeldeen
Copy link
Copy Markdown
Member

deyaaeldeen commented Jul 9, 2021

@sarangan12 and I talked offline and agreed on prioritizing fixing the samples URL issue since it is a correctness issue. I am fine fixing all other issues after this release, they're just about being consistent with data-plane packages. Keep up the great work guys, I am very excited for this upcoming release!

@dw511214992
Copy link
Copy Markdown
Member Author

dw511214992 commented Jul 12, 2021

@sarangan12 I have written a script to change homepage, so if it's not easy to fix it now in codegen side, you can postpone it to next release. thanks

@dw511214992 dw511214992 changed the title @azure/arm-resources track2 js sdk track2 @azure/arm-resources Jul 12, 2021
@dw511214992
Merge branch 'main' into dw/track2-arm-resource
2580d9e 
# Conflicts:
#	common/config/rush/pnpm-lock.yaml
#	rush.json
@dw511214992
update
e631839
deyaaeldeen
deyaaeldeen reviewed Jul 13, 2021
View reviewed changes
sdk/resources/arm-resources/src/lro/locationPolling.ts
* Changes may cause incorrect behavior and will be lost if the code is regenerated.
*/

// Copyright (c) Microsoft Corporation.
Copy link
Copy Markdown
Member

@deyaaeldeen deyaaeldeen Jul 13, 2021

Choose a reason for hiding this comment

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

This is weird to have both copyright banners, @sarangan12 should we remove the banner from the lro files?

deyaaeldeen
deyaaeldeen reviewed Jul 13, 2021
View reviewed changes
sdk/resources/arm-resources/tsconfig.json
"esModuleInterop": true,
"allowSyntheticDefaultImports": true,
"forceConsistentCasingInFileNames": true,
"preserveConstEnums": true,
Copy link
Copy Markdown
Member

@deyaaeldeen deyaaeldeen Jul 13, 2021

Choose a reason for hiding this comment

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

why is this needed if we do not generate const enums?

deyaaeldeen
deyaaeldeen approved these changes Jul 13, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

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

This iteration looks great! I guess all other PRs will look very similar to this one. @sarangan12 could you please look at them?

lirenhe
lirenhe reviewed Jul 15, 2021
View reviewed changes
sdk/resources/arm-resources/src/lro/azureAsyncPolling.ts
* Changes may cause incorrect behavior and will be lost if the code is regenerated.
*/

// Copyright (c) Microsoft Corporation.
Copy link
Copy Markdown
Member

@lirenhe lirenhe Jul 15, 2021

Choose a reason for hiding this comment

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

Not a blocking but it seems there are 2 MIT license in this file.

Copy link
Copy Markdown
Member Author

@dw511214992 dw511214992 Jul 15, 2021

Choose a reason for hiding this comment

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

yes. @deyaaeldeen has commented about it in #16170 (comment)

@qiaozha qiaozha merged commit 47e53cc into main Jul 15, 2021
@qiaozha qiaozha deleted the dw/track2-arm-resource branch July 15, 2021 11:30
deyaaeldeen added a commit that referenced this pull request Jul 26, 2021
@dw511214992 @deyaaeldeen @qiaozha
track2 @azure/arm-resources (#16170)
7224909 
* @azure/arm-resources track2

* Update sdk/resources/arm-resources/CHANGELOG.md

Co-authored-by: Deyaaeldeen Almahallawi <dealmaha@microsoft.com>

* update changelog, version in context and add license

* remove npm install in prepack

* update

* udpate pr

* update

* update

* update

* update changelog

* update changelog

Co-authored-by: Deyaaeldeen Almahallawi <dealmaha@microsoft.com>
Co-authored-by: qiaozha <qiaozha@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lirenhe lirenhe lirenhe left review comments

@deyaaeldeen deyaaeldeen deyaaeldeen approved these changes

@weshaggard weshaggard Awaiting requested review from weshaggard

@ramya-rao-a ramya-rao-a Awaiting requested review from ramya-rao-a

@praveenkuttappan praveenkuttappan Awaiting requested review from praveenkuttappan

@qiaozha qiaozha Awaiting requested review from qiaozha qiaozha is a code owner

@mikeharder mikeharder Awaiting requested review from mikeharder mikeharder is a code owner

@mitchdenny mitchdenny Awaiting requested review from mitchdenny

@witemple-msft witemple-msft Awaiting requested review from witemple-msft

+1 more reviewer

@sarangan12 sarangan12 sarangan12 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dw511214992 @weshaggard @sarangan12 @deyaaeldeen @lirenhe @qiaozha