Azure / azure-sdk-tools

Tools repository leveraged by the Azure SDK team.
MIT License
109 stars 166 forks source link

[Feature] Merging changes for data-plane specs in the azure-rest-api-specs repo (PR merge sign-off) (`PublishToCustomers` label) #7648

Open weshaggard opened 5 months ago

weshaggard commented 5 months ago

For data-plane spec PRs we want to move to a model where PR authors have permissions to merge their own pull requests once the checks are green.

For now, this will be the logic for data-plane only but eventually we will use the same strategy for mgmt-plane changes as well.

cc @johanste @mikekistler @rkmanda @konrad-jamrozik

mikekistler commented 5 months ago

Is there a check that blocks merge of a data-plane API with the new-api-version label without the ApiStewardshipBoard-signedOff label? We need that if it is not already in place.

JeffreyRichter commented 5 months ago

Instead of ReadyToPublish how about PublishForCustomers? This makes it clearer that customers will consume what they publish - they are really shipping this PR.

konrad-jamrozik commented 5 months ago

@JeffreyRichter +1 to your proposal. But I wanted to suggest a minor tweak: PublishToCustomers, or maybe even PublishToExternalCustomers instead of PublishForCustomers. According to this conversation with ChatGPT this will convey the meaning even stronger:

"Publish for customers" implies that the content is being created and published with customers as the intended audience. It focuses on the customers as the beneficiaries of the publication.

"Publish to customers" suggests the act of distributing or sending out the published content directly to customers. This phrase emphasizes the direction of the publication towards the customers.

Basically "Publish to external customers" sounds more scary IMO, and that's we are trying to convey here.

weshaggard commented 5 months ago

Is there a check that blocks merge of a data-plane API with the new-api-version label without the ApiStewardshipBoard-signedOff label? We need that if it is not already in place.

Yes the Automated merging requirements met check will ensure that this label is present as well as any other approval labels.

I'm fine with the label name change to PublishToCustomers, I've updated my proposal description.

JeffreyRichter commented 5 months ago

I'm fine with PublishToCustomers (I don't think "External" is necessary). Thanks Konrad & Wes.

mikekistler commented 4 months ago

This seems workable to me. We should give it a try -- and monitor carefully the first few weeks to determine if there are any unanticipated issues.

heaths commented 4 months ago

I think the API Stewardship Board should also go through and prioritize validation issues we think need to be resolved before we turn on the tap. Do we have a way to subdivide "Spec PR Tools" issues already? If not, might I suggest a project that adds category e.g., "Validation", and priority fields? I already see so many PRs that merge with failed non-required checks. I want to make sure people aren't merging without making sure everything we required is indeed required, and that any validation we think is missing is prioritized with, perhaps, P1s getting resolved first before opening up permissions.

konrad-jamrozik commented 4 months ago

Per my chat with @heaths, I triage issues with Spec PR Tools label: https://github.com/orgs/Azure/projects/198/views/9

And my backlog is: https://github.com/orgs/Azure/projects/198/views/10

I would suggest to create a new label like Required for data-plane auto-merge or similar, and add it to applicable issues.

konrad-jamrozik commented 4 months ago

@heaths let's include in the new label data-plane, because there is a separate thread of work for ARM TypeSpec auto-merge:

konrad-jamrozik commented 3 months ago

Relevant guidance that will need to be updated:

Relevant recent PR:

konrad-jamrozik commented 2 months ago

Temporary guidance update: