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

Agent spec process #304

Merged
merged 21 commits into from
Aug 11, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Raising a PR against the agent spec folder will automatically request reviews from all agent teams
# If the PR is not ready for review, create a draft PR instead
# See also docs/agents/spec-process.md
/docs/agents @elastic/apm-agent-go @elastic/apm-agent-java @elastic/apm-agent-net @elastic/apm-agent-node-js @elastic/apm-agent-php @elastic/apm-agent-python @elastic/apm-agent-ruby @elastic/apm-agent-rum
9 changes: 9 additions & 0 deletions .github/ISSUE_TEMPLATE/agents-discussion.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
name: Agents discussion
about: Creates a agent discussion issue
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
labels: agents, discussion
---

<!--
Once the discussion is at a s
-->
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
22 changes: 0 additions & 22 deletions .github/ISSUE_TEMPLATE/agents-poll.md

This file was deleted.

58 changes: 58 additions & 0 deletions docs/agents/spec-process.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
Every change that affects more than one agent should be initiated via a change to the specification rather than creating a cross-agent issue.
felixbarny marked this conversation as resolved.
Show resolved Hide resolved

## Advantages
felixbarny marked this conversation as resolved.
Show resolved Hide resolved

- Ensures we have an up-to-date [spec](https://www.joelonsoftware.com/2000/08/09/the-joel-test-12-steps-to-better-code/) for agents.
- Easier to keep track of what the current state of the proposal is.
- Comments to a specific sentence of the spec can be made inline,
just like in code reviews.
- If there are changes or amendments to a spec later on,
a PR to an existing spec is much cleaner than having to track down all preceding issues to find out what the current state is.
- Easier to get a new agent (in-house or community) started.

## Process

1. Agents discussion issue \
If there's a lot of uncertainty about what the poposal is,
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
a discussion issue should be created.
In doubt, lean towards not creating a discussion issue and start with a draft PR.
Discussion issues should not have votes/agent checkboxes.
It's not expected that all agents participate in the initial discussions, although everyone is invited to do so.
graphaelli marked this conversation as resolved.
Show resolved Hide resolved
1. Draft PR \
It's fine if the first draft of the spec change even if details are unclear and need discussion.
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
The PR should include which agents are affected by this change.
Specifically, it should always be clear whether the RUM agent is affected by this.
This doesn't have to be determined yet when creating the draft PR,
but it is required before marking the PR as `Ready for Review`.
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
1. Approval by at least one other agent \
This makes sure the change makes sense for not just one agent.
This doesn't ping all agent teams yet,
gregkalapos marked this conversation as resolved.
Show resolved Hide resolved
which only happens in the next step.
Of course,
every agent is more than welcome to contribute at any point in this process,
but they are not expected to actively keep track if it.
1. Ready for Review \
This automatically requests review from all agents due to the [`CODEOWNERS`](https://github.com/elastic/apm/tree/master/.github/CODEOWNERS) file.
After a quorum of agents has approved, others have 1w to veto by requesting changes.
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
If the proposed changes are likely to be problematic for a certain agent,
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
an approval from that agent is mandatory.
1. Prioritization \
At any point in time,
but before merging the PR,
the priority should be determined.
That happens in collaboration with devs and leads.
It's recommended to do that sooner rather than later
so that we don't spend a lot of time discussing a feature that's not going to be prioritized.
As a result of the prioritization,
the PR gets assigned to a milestone,
reflecting the stack release version this feature should be implemented by all agents.
If some agents are not going to implement the change by that stack release version,
or if there are no immediate plans to implement the change at all,
a comment should be made on the PR.
1. PR can be merged by the author of the PR \
Three business days before merging, there should be a last call for objections.
1. The author of the PR creates an issue in each agent's repo \
felixbarny marked this conversation as resolved.
Show resolved Hide resolved
They make sure the issues are assigned to a stack release milestone,
based on the milestone of the spec PR.
Agent teams can still change the milestone if priorities shift.
However, they should talk to their team lead about it.
felixbarny marked this conversation as resolved.
Show resolved Hide resolved