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

Support policy checks #69

Merged
merged 28 commits into from
Feb 26, 2024
Merged

Support policy checks #69

merged 28 commits into from
Feb 26, 2024

Conversation

bufdev
Copy link
Member

@bufdev bufdev commented Feb 13, 2024

No description provided.

@nicksnyder
Copy link
Member

I am taking at pass at this and will push up a proposed change.

CommitReviewState is a better name because LabelReviewState seems to imply a label has only one such state. The number of review states is 1:1 with number of commits (not labels).

Update docs to clarify that review state information is returned depending on whether review state was enabled at the time of push for a given commit (not whether it is enabled currently).
Copy link
Member

@nicksnyder nicksnyder left a comment

Choose a reason for hiding this comment

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

I renamed LabelReviewState to CommitReviewState because review states are more closely bound to Commits than Labels (they are 1:1 with the former and Many:1 with the latter). Also cleaned up some docs.

Before merging, I think we should decide how we want to consistently refer to this feature because that might impact names like review_enabled, but I started a Slack thread about that.

buf/registry/module/v1beta1/label.proto Outdated Show resolved Hide resolved
buf/registry/module/v1beta1/label.proto Outdated Show resolved Hide resolved
buf/registry/module/v1beta1/label_service.proto Outdated Show resolved Hide resolved
buf/registry/module/v1beta1/label_service.proto Outdated Show resolved Hide resolved
@nicksnyder nicksnyder changed the title Add BreakingState and ReviewState to ListLabelHistoryResponse Support governance workflow Feb 16, 2024
@nicksnyder nicksnyder changed the title Support governance workflow Support policy checks Feb 16, 2024
Copy link
Member

@nicksnyder nicksnyder left a comment

Choose a reason for hiding this comment

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

Ok, after iterating on this a bit and discussing some things around naming with Mary, the current state of this PR is my proposal.

"Governance workflow" can/will be the umbrella of features that include policy checks, but also include things that aren't necessarily related to approving changes (e.g. audit logs, permissions, etc). An analogy here is that this is "Compliance".

"Policy checks" are specifically checks that run to determine if a commit can be placed on a label's history. An analogy here is "CI jobs" (which must pass before merging).

PolicyChecksState is the overall state of all policy checks for a given Commit x Label. Currently we have one implied check: breaking change detection. In the future we could have more (e.g. lint), but there would still be a single top level status (the one already proposed here). In the future we could add lower level status for individual policy checks (e.g. so you can know the specific status for breaking change detection, or lint, or otherwise), but we can add that to PolicyChecksState if/when we need it.

Other changes

  • Expanded the types to support dependencies between policy checks (which we need because of workspace push).
  • Standardize on "Approved" instead of "Accepted" since the former seem more likely what we would want to use in the UI (we already use this today, and this is what code review tools use so it seems like the right term of art)

I am out next week, so if everyone else is ok with this, please go ahead and merge.

@bufdev bufdev merged commit 641cb69 into main Feb 26, 2024
6 checks passed
@bufdev bufdev deleted the breaking-review-labels branch February 26, 2024 23:34
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.

5 participants