Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Skip validating MSI in PatchBodyParametersSchema #762

Merged
merged 9 commits into from
Feb 5, 2025

Conversation

AkhilaIlla
Copy link
Collaborator

This PR skips validation for MSI(managed service identity) as it is being referenced from common-types & has the required field & is being referenced in patch body parameter schema by several RP's & are having to get an exception. So, skipping to avoid that step.


// skip validation for MSI(managed service identity),
// as it is being referenced from common-types
if (parameters.schema.description && parameters.schema.description.includes("Managed service identity")) {
Copy link
Contributor

@tejaswiMinnu tejaswiMinnu Jan 21, 2025

Choose a reason for hiding this comment

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

Shouldn't we think about case-sensitive scenarios? I know we're checking from common-types so it shouldn't change.
But why don't we check the parameter name ("ManagedServiceIdentity") instead of the description? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because its being referenced, the param name wouldnt be available in the schema

Copy link
Member

Choose a reason for hiding this comment

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

pl use something more deterministic than description. The check should perhaps be done on the properties of the model?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated.

Copy link
Member

@rkmanda rkmanda left a comment

Choose a reason for hiding this comment

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

🕐

Copy link
Contributor

@tejaswiMinnu tejaswiMinnu left a comment

Choose a reason for hiding this comment

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

This branch has some conflicts. Can you take a look and resolve them?

Copy link
Member

@mikeharder mikeharder left a comment

Choose a reason for hiding this comment

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

Please add a brief description to https://github.com/Azure/azure-openapi-validator/blob/main/packages/rulesets/CHANGELOG.md.

I already bumped the version to 2.1.6, so you (or I) can publish 2.1.6 whenever you want these changes to go live.

@mikeharder
Copy link
Member

mikeharder commented Jan 25, 2025

This branch has some conflicts. Can you take a look and resolve them?

It needed to merge from main, but I didn't see any conflicts. #Resolved

@mikeharder mikeharder dismissed their stale review January 25, 2025 01:14

Changes requested but not required

@AkhilaIlla
Copy link
Collaborator Author

remember Konrad saying he made some changes which will no longer need these changelogs to be added.
I can add one now.


In reply to: 2573868553

@AkhilaIlla
Copy link
Collaborator Author

I dont see any conflicts. Pulled latest changes too.


In reply to: 2573813049

@AkhilaIlla
Copy link
Collaborator Author

@rkmanda can you re-review please.


In reply to: 2568197148

@mikeharder
Copy link
Member

remember Konrad saying he made some changes which will no longer need these changelogs to be added. I can add one now.

In reply to: 2573868553

We don't use the rush changelog feature anymore, but it would be nice to just manually update changelog.md when making changes. I pushed this to your PR.

Copy link
Member

@rkmanda rkmanda left a comment

Choose a reason for hiding this comment

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

:shipit:

@AkhilaIlla AkhilaIlla merged commit 576ae87 into main Feb 5, 2025
6 checks passed
@AkhilaIlla AkhilaIlla deleted the akhilailla/fix_PatchBodyParametersSchema branch February 5, 2025 20:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants