Skip to content

Conversation

@jlichwa
Copy link
Contributor

@jlichwa jlichwa commented Aug 15, 2019

Event definitions for Key Vault Certificates, Keys and Secrets - NewVersion, Near Expiry and Expired

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

ARM API Review Checklist

  • Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
  • adding/removing APIs.
  • adding/removing properties.
  • adding/removing API-version.
  • adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

  • If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
    Please follow the link to find more details on API review process.

Event definitions for Key Vault Certificates, Keys and Secrets - NewVersion, Near Expiry and Expired
@jlichwa jlichwa requested a review from kalyanaj as a code owner August 15, 2019 23:08
@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Aug 15, 2019

In Testing, Please Ignore

[Logs] (Generated from 44674ab, Iteration 12)

Warning Go: test-repo-billy/azure-sdk-for-go [Logs] [Diff]
  • Warning eventgrid/2018-01-01 [Logs]
Succeeded Python: test-repo-billy/azure-sdk-for-python [Logs] [Diff]
Failed JavaScript: test-repo-billy/azure-sdk-for-js [Logs] [Diff]
Warning Ruby: test-repo-billy/azure-sdk-for-ruby [Logs] [Diff]
  • No packages generated.

@AutorestCI
Copy link

AutorestCI commented Aug 15, 2019

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#6818

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Aug 15, 2019

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#5701

@mmyyrroonn
Copy link
Contributor

@jlichwa Hello. Why do we have this PR? I don't see any path referring these definitions. We cannot pass the Avocado's check.

Copy link
Contributor

@kalyanaj kalyanaj left a comment

Choose a reason for hiding this comment

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

For consistency with the rest of the files under EventGrid dataplane, please consider removing the additional line spacing used in this file.

Copy link
Contributor

@kalyanaj kalyanaj left a comment

Choose a reason for hiding this comment

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

For each definition name such as CertificateNewVersionCreated, KeyNearExpiry etc. since all the definitions share the same namespace Microsoft.EventGrid.Models, please add the KeyVault prefix, as an example, please rename CertificateNewVersionCreated to KeyVaultCertificateNewVersionCreated. This will help with better discoverability of all KeyVault specific event definitions. Also, typically, we use the "EventData" suffix for such event definitions (e.g. StorageBlobCreatedEventData), so please consider adding EventData suffix as well to each definition name. (for example "CertificateNewVersionCreated" would become KeyVaultCertificateNewVersionCreatedEventData.).

@jlichwa
Copy link
Contributor Author

jlichwa commented Aug 22, 2019

Updated names KeyVault prefix and Event Data Suffix

@mmyyrroonn
Copy link
Contributor

@jlichwa Can I assume that this PR is in progress and there will be another team working on the API paths later?

@jlichwa
Copy link
Contributor Author

jlichwa commented Aug 26, 2019 via email

@jlichwa
Copy link
Contributor Author

jlichwa commented Aug 27, 2019

For consistency with the rest of the files under EventGrid dataplane, please consider removing the additional line spacing used in this file.

Done

@jlichwa
Copy link
Contributor Author

jlichwa commented Aug 28, 2019

@myronfanqiu Can we Merge based on comments? @kalyanaj

@jlichwa jlichwa closed this Aug 28, 2019
@jlichwa jlichwa reopened this Aug 28, 2019
@mmyyrroonn
Copy link
Contributor

@jlichwa I still don't understand the purpose of this PR. Who will use these definitions?

@jlichwa
Copy link
Contributor Author

jlichwa commented Aug 29, 2019

@jlichwa I still don't understand the purpose of this PR. Who will use these definitions?

It is for EventGridTeam( @kalyanaj ) as part of their OnBoarding process for new events schema. Based on that schema they will build API (they cannot do it without it) and once it is ready for Public Preview, it will be added to Readme.md as a part of API to customers.
My understanding is just more about Teams separation, EventGrid customers, provide the schema to EventGrid dev team (which is that json file I added) and based on that EventGrid team build API and enable that events schema definitions to public - so 2 teams contribute to the entire feature.

@kalyanaj
Copy link
Contributor

@myronfanqiu, please see my comment earlier in this PR (#6975 (comment)) for more context on how these definitions will be used. (FYI: @jlichwa).

Copy link
Contributor

@kalyanaj kalyanaj left a comment

Choose a reason for hiding this comment

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

LGTM.

@mmyyrroonn mmyyrroonn added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 30, 2019
@sarangan12
Copy link
Contributor

@jilchwa One comment. You have created a new JSON file in this PR. But this file is never included in the readme.md file. So, no tests will be run on this file unless you actually include it. I believe the failed tests are really unrelated to your file. (which is a seperate discussion). But, first please include the file reference in the readme file.

@mmyyrroonn mmyyrroonn removed the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 31, 2019
@mmyyrroonn
Copy link
Contributor

Remove the label WatiforArmFeedback since it's related with data-plane

@mmyyrroonn
Copy link
Contributor

@jlichwa Hello. Resolve the CI and I can merge this PR.

@yungezz yungezz merged commit 36db3ff into Azure:master Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants