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

Enforce specs for cross-agent features #192

Closed
felixbarny opened this issue Jan 8, 2020 · 7 comments
Closed

Enforce specs for cross-agent features #192

felixbarny opened this issue Jan 8, 2020 · 7 comments

Comments

@felixbarny
Copy link
Member

Instead of issue descriptions acting as specs, I'd like to do this in the form of a PR which adds a new file to https://github.com/elastic/apm/tree/master/docs (or /specs?).

This has the following advantages:

  • Ensures we have an up-to-date spec 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

Current spec changes which could benefit from this are #115 and #180. The conversion would be as easy as copy/pasting the issue descriptions to markdown files. Further down, we could consolidate on how a spec is structured (motivation, description, acceptance tests etc.).

@felixbarny
Copy link
Member Author

@elastic/apm-agent-devs @graphaelli @axw could I get your opinion on this? If you're fine with it, I would convert this to a dedicated markdown file in https://github.com/elastic/apm/tree/master/docs/agents.

@axw
Copy link
Member

axw commented Jul 16, 2020

SGTM @felixbarny. Did #270 remind you of htis? 😅 (I think that one deserves a spec, but needs more details fleshed out.)

@felixbarny
Copy link
Member Author

Did #270 remind you of htis? 😅

Possibly 😇

I'm also not terribly sure what the best workflow might be. I think even if details are unclear, it may be beneficial to start a discussion about a change by creating a PR with proposed changes. It has the advantage that as things become clearer and as the initial PR is revised, the current diff always summarizes the current state of discussions. This makes it easier to someone to jump in without having to compute a mental diff by parsing all the comments on an issue.

It also makes it easier to comment on a specific line of the proposal and resolving comments removes clutter in the thread of discussion.

Potentially, we could also remove the checkboxes in favor of PR approvals. And only after a proposal gets merged, we would create an implementation issue that tracks the progress for different agents. Ideally, there would already be one or two reference implementations for a change.

@axw
Copy link
Member

axw commented Jul 16, 2020

Potentially, we could also remove the checkboxes in favor of PR approvals. And only after a proposal gets merged, we would create an implementation issue that tracks the progress for different agents. Ideally, there would already be one or two reference implementations for a change.

That sounds like a reasonable idea to me. Perhaps use CODEOWNERS to keep it low overhead.

@felixbarny
Copy link
Member Author

Perhaps use CODEOWNERS to keep it low overhead.

Great idea!

You can use a CODEOWNERS file to define individuals or teams that are responsible for code in a repository.
...
Code owners are automatically requested for review when someone opens a pull request that modifies code that they own. Code owners are not automatically requested to review draft pull requests.

In the initial phase when there's a lot of uncertainty about the details of a proposal, we could work with draft PRs to avoid "spamming" all agent teams with PR requests while still welcoming everyone to participate in discussions if they want to.

@basepi
Copy link
Contributor

basepi commented Jul 16, 2020

I think this is a great idea. Central location is a must for keeping things consistent, especially as we add new agents in the future.

@felixbarny
Copy link
Member Author

superseded by #304

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

No branches or pull requests

3 participants