-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
redis: adding the missing notify-keyspace-events
value
#22407
redis: adding the missing notify-keyspace-events
value
#22407
Conversation
Hi, @tombuildsstuff Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. vscswagger@microsoft.com |
Swagger Validation Report
|
Compared specs (v0.10.7) | new version | base version |
---|---|---|
redis.json | 2023-05-01-preview(159377b) | 2023-05-01-preview(main) |
redis.json | 2021-06-01(159377b) | 2021-06-01(main) |
redis.json | 2022-05-01(159377b) | 2022-05-01(main) |
redis.json | 2022-06-01(159377b) | 2022-06-01(main) |
redis.json | 2023-08-01(159377b) | 2023-08-01(main) |
️️✔️
Breaking Change(Cross-Version) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️
CredScan succeeded [Detail] [Expand]
There is no credential detected.
️🔄
LintDiff inProgress [Detail]
️️✔️
Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️❌
SwaggerAPIView: 0 Errors, 0 Warnings failed [Detail]
️️✔️
TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️
ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️
SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️
PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️
SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️
Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️
PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️
Automated merging requirements met succeeded [Detail] [Expand]
Swagger Generation Artifacts
|
Thank you for your contribution tombuildsstuff! We will review the pull request and get back to you soon. |
Generated ApiView
|
NewApiVersionRequired reason: |
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.
Technically we could also have a validation regex for allowed values, but that's something that depends on the redis version, which will be hard to model and keep up to date... |
Hi @tombuildsstuff, one or multiple breaking change(s) is detected in your PR. Please check out the breaking change(s), and provide business justification in the PR comment and @ PR assignee why you must have these change(s), and how external customer impact can be mitigated. Please ensure to follow breaking change policy to request breaking change review and approval before proceeding swagger PR review. |
specification/redis/resource-manager/Microsoft.Cache/stable/2022-06-01/redis.json
Outdated
Show resolved
Hide resolved
@dw511214992 since this has been signed off, what's needed to get this merged? |
@tombuildsstuff @TimLovellSmith Why was the label |
Sory for the lack of progress. I think we're all happy with this as a bugfix to docs from an ARM perspective, its just breaking change / versioning analysis needed. |
@TimLovellSmith given there's a new version of the Redis API on a frequent enough basis, if it's easier to add support for this to the new API Version and then just focus on that new version, that's also fine with me fwiw - backporting this to previous versions is mostly to ensure the Swagger definitions are correct, more than anything. |
@JeffreyRichter Do you think this contribution is okay as is, as a breaking change bugfix? |
@konrad-jamrozik Who does versioning review? |
@TimLovellSmitth per https://aka.ms/azsdk/pr-diagram this is handled by Breaking Change Review Board (step 1). But the PR author needs to create an intake form first, per the instructions. However, given @tombuildsstuff is not MSFT and this process is MSFT-only, let me ping appropriate people. @mikekistler this PR needs review but the PR author cannot file intake form as they are non-MSFT. Can you have a look? It has both |
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
If the service versions actually supported this over the wire but it was missing from the swagger, then we can approve the breaking change as a "BugFix". |
Yes, that is the case. |
@tombuildsstuff this PR is from over a year ago so the automation didn't add You can also consult https://aka.ms/azsdk/pr-diagram I just ensured your PR will be on ARM review queue. Looks like your PR is first. Hence ARM on-call should review this PR soon and provide feedback or approve the ARM review. |
Whilst this value can currently be defined via the additionalProperties in the Track1 Go SDK, since it's included in the description presumably this should also be an explicitly defined field? Description:
As it stands today this field can be set via adding this to/parsing this from the dictionary, however since other properties have been defined explicitly, presumably this one should be too?
More details: