Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

CI: Require pullapprove ack for protocol changes #176

Merged

Conversation

jodh-intel
Copy link
Contributor

A change to any of the gRPC protocol buffer files (*.proto) should
require an additional approval due to the potential impact it could
have across the system.

Fixes #175.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

@jodh-intel jodh-intel force-pushed the pullapprove-for-grpc-changes branch from e8bd995 to 7cdd77f Compare March 19, 2018 09:22
@jodh-intel
Copy link
Contributor Author

We might want to tweak the list of teams I've added here. Here's my rationale for the currently listed team for this change:

Team Rationale
architecture-team Should be aware of any architectural changes.
builder Need to be aware of such changes to potentially create a new image version containing the new agent.
packaging Need to be aware of any system-breaking protocol change (to allow new packages to be generated [once we are actually producing packages]).

@sboeuf
Copy link

sboeuf commented Mar 19, 2018

LGTM

Approved with PullApprove

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

lgtm
one note.

.pullapprove.yml Outdated
@@ -41,3 +41,16 @@ groups:
- "*.md"
exclude:
- "vendor/*"

protocol-changes:
required: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

required: 1 feels a little slim to me - if it is architectural change then I would not be against increasing this .

Copy link

Choose a reason for hiding this comment

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

Oh yeah I've missed that actually. @jodh-intel could you please put 2 or 3 here ?
I think 2 is already the default for the agent repo, isn't it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha - I put in 1 as I was expecting folk to fight against >1 ;)

I''ve now changed it to 2. The ideal in my mind would be 1 from the arch team and 1 from one of the other two teams. However, fwics, that would require a bit of extra bloat in the config as we'd need two "protocol-changes" groups - one just for the arch team and another for the other two teams.

Copy link

@sboeuf sboeuf Mar 19, 2018

Choose a reason for hiding this comment

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

2 is fine for now, we'll introduce protocol-changes group in a follow-up PR I guess

A change to any of the gRPC protocol buffer files (`*.proto`) should
require two additional approvals due to the potential impact it could
have across the system.

Fixes kata-containers#175.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@jodh-intel jodh-intel force-pushed the pullapprove-for-grpc-changes branch from 7cdd77f to 5e6c385 Compare March 19, 2018 14:15
@laijs
Copy link
Contributor

laijs commented Mar 20, 2018

LGTM

Approved with PullApprove

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants