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

Initial enhancement proposal template #10539

Merged

Conversation

npolshakova
Copy link
Contributor

Description

Adds a template for enhancement proposals

API changes

None

Code changes

None

CI changes

None

Docs changes

  • Adds template for creating Enhancement Proposals (EP)

Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

A couple of quick EOD thoughts. Everything LGTM at a high-level. Thanks for taking a stab at all of this.

-->

<!-- toc -->
- [Background](#Background)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the background link is broken -- at least when I look at the markdown preview tab in the UI.

Copy link
Member

Choose a reason for hiding this comment

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

Also, do we want a table of contents of proposals? I always felt it was overkill in the KEP format and we could slim down while still following the high-level sections (e.g. background, motivation, goals / non-goals) without being too boggle down on a heavy format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah fair, there are only a couple sections in the template if we remove the implementation details subsections:

<!-- toc -->
- [Background](#background)
- [Motivation](#motivation)
    - [Goals](#goals)
    - [Non-Goals](#non-goals)
- [Implementation Details](#implementation-details)
    - [Test Plan](#test-plan)
- [Alternatives](#alternatives)
- [Open Questions](#open-questions)
<!-- /toc -->

I'm down to remove it all together.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think I'm leaning towards removing it all together too.

and make progress.
-->

## Implementation Details
Copy link
Member

Choose a reason for hiding this comment

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

If I was introducing a new CRD or some extension of an API (e.g. aggregated webhook), would I outline that within the implementation details section or a dedicated API section?

Specify any additional frameworks or tools required for testing.
-->

## Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

What happens if there's a legitimate alternative worth exploring further? Create another design doc and link back here? Add a sub-section under this alternative with a lengthy amount of context/description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I think you could link other EPs here or have a length discussion section. Really depends on the feature!

proposal will be implemented, this is the place to discuss them.
-->

### Configuration
Copy link
Member

Choose a reason for hiding this comment

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

At a glance, these high-level sub-sections seem reasonable to me. I wonder whether we could get away with a high-level "Implementation Details" section without requiring predefined sub-sections to provide some more flexibility in structuring and make the EP process a bit less prescriptive. Just thinking aloud -- no strong opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good call- maybe I'll just list the subsection in the comment as a reminder of what sections can be included? This probably means we can also remove the index?

Copy link
Member

Choose a reason for hiding this comment

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

I think a comment is a good middle ground as you're still guiding the author towards adding color around the major sub-systems in the project.

subsequent PRs.

Just because a EP is merged does not mean it is complete or approved. Any EP
marked as `provisional` is a working document and subject to change. You can
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe worth mentioning what possible states the EP can be in (provisional, implemented, etc?), and how you mark it as 'provisional' (maybe we need another doc for that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, good call- maybe I should remove the "provisional" section? The EP has two stages:

  1. provisional (has not been fully implemented yet)
  2. implemented (code has merged, EP is up to date to reflect the state)

Maybe this should be a label/field on the issue itself?


<!--
This section should contain enough information that the specifics of your
change are understandable. This may include API specs (though not always
Copy link
Contributor

Choose a reason for hiding this comment

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

how should we handle API/design/implementation updates, would this design doc need to be kept up to date whenever those change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think significant API, design, or implementation changes should have a dedicated EP linked to a specific issue, which is closed once the code merges. There are two cases:

  1. If code changes affect the API/design during the implementation stage, the EP must be updated. Before merging the final PR or closing the issue, the EP should reflect the latest state.
  2. If there are changes that need to be made after the code merges, a new EP/issue needs to be created. This can reference the old issue, but the original EP should remain unchanged.

any review.
-->

* Issue: [#ID](URL to GitHub issue)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to keep these issues open as the enhancement graduates from alpha -> beta -> stable (like KEPs), or close them once implemented?

if so, should we add something about the current stage / graduation criteria in this template (maybe that's getting ahead of ourselves)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need specific stages, but that's a good question on when an EP can be closed out. I think for now though the issue should be closed out when the code merges, and then any larger follow-ups require their own EP.

@jenshu jenshu added this pull request to the merge queue Jan 30, 2025
Merged via the queue into kgateway-dev:main with commit b04118c Jan 30, 2025
8 checks passed
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.

3 participants