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

Swaggers omit x-ms-enum which causes spurious diff's #21894

Open
tombuildsstuff opened this issue Aug 20, 2021 · 9 comments
Open

Swaggers omit x-ms-enum which causes spurious diff's #21894

tombuildsstuff opened this issue Aug 20, 2021 · 9 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@tombuildsstuff
Copy link
Contributor

Unfortunately the Azure Swagger quality is pretty variable, and whilst the majority of constants use x-ms-enum, some others don't. This leads to weird naming in the SDK as we've raised previously such as Enum42 - which is an issue across the Azure SDK's.

As requested in Azure/azure-sdk-for-go#2182 - is there a plan to fix this data quality issue?


The original comment in Azure/azure-sdk-for-go#2182:

@RickWinter reading through the original comment this appears to be more regarding the Enums changing names across versions (than the validation mentioned in Joel's comment, where this got renamed) - for which the root-cause appears to be missing x-ms-enums in the Swaggers - whilst this could be fixed in this SDK, is there a plan to go through and add the missing x-ms-enums to the Swaggers, which would fix this?

Originally posted by @tombuildsstuff in Azure/azure-sdk-for-go#2182 (comment)

@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Aug 20, 2021
@RickWinter RickWinter added the Client This issue points to a problem in the data-plane of the library. label Aug 25, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Aug 25, 2021
@jhendrixMSFT
Copy link
Member

The problematic naming cited above is due to how the track 1 modeler works. This has been fixed in track 2.

Adding x-ms-enum at this point will introduce breaking changes in track 1 (albeit with improved names) and track 2 (different, though possibly improved) names.

@ghost
Copy link

ghost commented Aug 25, 2021

Hi @tombuildsstuff. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text “/unresolve” to remove the “issue-addressed” label and continue the conversation.

@ghost
Copy link

ghost commented Aug 25, 2021

Hi @tombuildsstuff. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text “/unresolve” to remove the “issue-addressed” label and continue the conversation.

@tombuildsstuff
Copy link
Contributor Author

@jhendrixMSFT I get that's been fixed in Track2, but I was specifically wondering regarding the data quality issue rather than fixing this in the code generator - since that would also fix the larger issue with names such as OdataTypeBasicMetricAlertCriteriaOdataTypeMicrosoftAzureMonitorSingleResourceMultipleMetricCriteria too for downstream consumers too.

To be honest I think this is tooling missing from the azure-rest-api-specs repository - perhaps that'd be a worthwhile investment here, intentionally not being a part of autorest but instead just checking these singular items for speed purposes - but running that inside of a Github Action would solve this. Perhaps this'd make more sense on that repository in retrospect?

@jhendrixMSFT
Copy link
Member

I believe there's been talk about adding a linter rule to enforce the inclusion of x-ms-enum on enum types. @nickzhums do you know anything about this?

@ghost
Copy link

ghost commented Sep 2, 2021

Hi @tombuildsstuff, since you haven’t asked that we “/unresolve” the issue, we’ll close this out. If you believe further discussion is needed, please add a comment “/unresolve” to reopen the issue.

@ghost ghost closed this as completed Sep 2, 2021
@tombuildsstuff
Copy link
Contributor Author

/unresolve as this is still pending

@ghost ghost reopened this Sep 2, 2021
@ghost ghost added the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Sep 2, 2021
@nickzhums
Copy link
Contributor

@lirenhe this seems an engineering issue - could you help to investigate?

@tombuildsstuff
Copy link
Contributor Author

@RickWinter to clarify - this issue isn't specifically regarding Track1 - Track2 merely works around it - this is a data issue which needs to be fixed within the Swagger itself. I'd suggest moving this issue to the Swagger repository but issues on that repository seem to be ignored.

Although a linting rule would go some way towards fixing this, since this is a soft-requirement it's possible for teams to ignore the linters which brings us back to where we are today - instead I'd argue that Enum Names should be explicitly defined by the Service Team to highlight their intention and as such should be a hard-requirement instead.

If these are pre-defined by the Service Team (and it's discouraged, but still possible to rename them across API versions) this would mean these are considered ahead of time - rather than changing as they do today (since at least in Track1 these can come from the Json Field Name in some cases).

Whilst that may seem like trivial difference, it puts the onus on the Service Team to give a canonical name to this Enums, which makes it less likely (but still possible) to change over time - meaning that users of the (any of the) Azure SDK's don't need to dig in and identify this themselves upon upgrading between different API versions.

Hopefully that helps clarify that a little further, but let me know if there's anything else :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

No branches or pull requests

5 participants