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

Eventgrid arm package is not packaged due to matching service and dat… #14301

Conversation

praveenkuttappan
Copy link
Member

arm-eventgrid package is not packed when building arm packages. Upon investigation I have noticed that eventgrid service path ./sdk/eventgrid itself skipped when traversing all paths in SDK root path so gulp packing step is not finding arm-eventgrid as an eligible package to be packed.

Reason: service name and data plane name is same in this case for eventgrid ( eventgrid). All data plane package names are considered as folders to be ignored when traversing paths and this logic doesn't check whether current folder is a package root or not. So it skips traversing ./sdk/eventgrid path further which causes to ignore arm-eventgrid also.

Solution: Ensure that path is package root when skipping that folder from further traversal.

@qiaozha
Copy link
Member

qiaozha commented Mar 16, 2021

fix issue #14302

Copy link
Member

@qiaozha qiaozha left a comment

Choose a reason for hiding this comment

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

LGTM

@praveenkuttappan praveenkuttappan added the Central-EngSys This issue is owned by the Engineering System team. label Mar 16, 2021
Comment on lines +116 to +118
if (!contains(folderNamesToIgnore, getName(folderPath))) {
// If current path is not in folder to ignore list then process it.
return true;
Copy link
Member

Choose a reason for hiding this comment

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

I think this returns true for the track 1 perf test packages since it isn't a rush package.
Track 1 perf test packages should also be ignored.
Logged a new issue to handle this case #14304.

Copy link
Member Author

Choose a reason for hiding this comment

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

@HarshaNalluru : Do we need any more change in this PR or is it good to merge?

Copy link
Member

Choose a reason for hiding this comment

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

This looks good. (perf track 1 can be handled in a separate PR.)

But I think if you can elaborate in comments with examples on what gets filtered at each step, it would help a lot.

@praveenkuttappan
Copy link
Member Author

@ramya-rao-a @jeremymeng : Can one of you please have a look at this PR and sign off too if this change looks fine?

@@ -112,14 +112,31 @@ function isPackageFolderPath(folderPath: string, packagesToIgnore: string[]): bo
return result;
}

function shouldProcessFolderPath(folderPath: string, folderNamesToIgnore: string[]): boolean {
if (!contains(folderNamesToIgnore, getName(folderPath))) {
Copy link
Member

@HarshaNalluru HarshaNalluru Mar 19, 2021

Choose a reason for hiding this comment

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

It looks harder to understand.
Would it help better if you add a comment above with an example?
Such as..

folderNamesToIgnore = [ "service-bus", "eventgrid", ... ]; // Coming from /sdk/service/<service-sdk-name> 
folderPath = "..../sdk/servicebus/service-bus"
getName(folderPath) = "service-bus" 

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, at other places too.

@qiaozha
Copy link
Member

qiaozha commented Mar 23, 2021

Could we get this PR merged now ?

@praveenkuttappan praveenkuttappan merged commit 07e2987 into Azure:master Mar 23, 2021
jay-most pushed a commit to jay-most/azure-sdk-for-js that referenced this pull request Apr 26, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Jun 8, 2021
Add configuration to generate AppPlatform SDK for API version 2021-06-01-preview (Azure#14301)

* Add configuration to generate AppPlatform SDK for API version 2021-03-03-preview

* Update AutoRest configuration after api-version change in Azure#14323
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Central-EngSys This issue is owned by the Engineering System team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants