-
Notifications
You must be signed in to change notification settings - Fork 522
[NE-2195] Add initial add-enhancement command #1870
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
[NE-2195] Add initial add-enhancement command #1870
Conversation
|
| - Ensure the area directory exists under `enhancements/` | ||
| - Create a valid filename from the name (lowercase, replace spaces with dashes) | ||
| - Verify all required YAML metadata is present | ||
| - Verify the JIRA ticket URL is included in the tracking-link metadata field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps add Run make verify to satisfy lint checks? -- I think it has an environment to handle this, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huum, good call. I was going to do it, but sadly the make lint was created for podman only, and I didn't wanted to change/fix on this PR (to not mix things).
Maybe should be done as a followup:
- Fix makefile/make it more generic
- Add to the command to run a
make lint
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, maybe add an issue for the follow-up and link here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bentito
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a bunch of nits, let me know what you think or make changes as you feel needed, and I'll smash the approve button.
|
added some fixes, commented on other review suggestions |
|
/lgtm |
Miciah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/hold
in case you want to address any of my comments first. Feel free to /hold cancel.
|
|
||
| ## Instructions | ||
|
|
||
| Act as an experienced software architect to create a comprehensive enhancement proposal. Follow these steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I wonder whether this "Act as an experienced software architect" phrase contributes to the confidently wrong nature of the output. Maybe, "Act as a competent, experienced software architect with impostor syndrome"? (Ha ha, only serious.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, my worry here is that whatever we add as a context will be the used context, so adding something like impostor syndrome will just add wrong things as a question instead of as a conclusion :D and I know exactly how it is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. It wasn't an entirely serious suggestion, coaxing an LLM not to be overconfidently wrong is probably requires more finesse than that, and it makes sense to prioritize addressing the problem from the other side (that is, cultivating a healthy skepticism of LLM output).
.claude/commands/add-enhancement.md
Outdated
| - Specific user stories or motivations if not clear from the description | ||
| - Explicit Goals or Non-Goals the user wants included | ||
| - Any specific technical constraints or requirements | ||
| - Topology considerations (Hypershift, SNO, MicroShift relevance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can wait for yours to be merged first, so this gives more context to claude on what is OKE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have requested some OKE consideration as well (look at my last commit)
.claude/commands/add-enhancement.md
Outdated
| - **Workflow Description**: Detailed workflow with actors and steps | ||
| - **Mermaid Diagram**: Add a sequence diagram when applicable to visualize the workflow | ||
| - **Metadata**: Fill in creation-date with today's date (2025-10-21), set other fields to TBD | ||
| - **API Extensions**: Only fill this section if the user confirms the proposal adds/changes CRDs, admission and conversion webhooks, aggregated API servers, or finalizers. Otherwise, add a TODO comment asking the user to complete this section if applicable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we also have ValidatingAdmissionPlugin and MutatingAdmissionPlugin as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added as part of my last commit
.claude/commands/add-enhancement.md
Outdated
| - Focus on the essential information | ||
| - Use bullet points and structured formatting | ||
| - Avoid unnecessary verbosity | ||
| - **Line Length**: Keep lines in the generated enhancement at a maximum of 80 characters. It is acceptable to exceed by 10-15 characters when necessary (e.g., for URLs or code examples), but not more than that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens with a URL that exceeds 95 characters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me test here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enforced the requirement on my last commit
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add refinement to stop breaking lines if it turns them invalid (URL, code), to support OKE, and some other refinements on how the enhancement should be generated
269b20e to
cda54c9
Compare
|
@rikatz: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| - **API Extensions**: Only fill this section if the user confirms the proposal adds/changes CRDs, admission and conversion webhooks, aggregated API servers, or finalizers. Otherwise, add a TODO comment asking the user to complete this section if applicable. | ||
| - **Implementation Details/Notes/Constraints**: Provide a high-level overview of the code changes required. Follow the guidance from the template: "While it is useful to go into the details of the code changes required, it is not necessary to show how the code will be rewritten in the enhancement." Keep it as an overview; the developer should fill in the specific implementation details. | ||
| - **Metadata**: Fill in creation-date with today's date, tracking-link with the provided JIRA ticket URL, set other fields to TBD | ||
| - **Mermaid Diagram**: Add a sequence diagram when the workflow involves multiple actors or complex interactions between components (e.g., user -> API server -> controller -> operator). Simple single-actor workflows may not need a diagram. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the model going to interpret that example as you expect it to? I'm curious because the example represents multiple actors but not complex interactions, and it is ambiguous whether it is an example of "multiple actors or complex interactions between components" or an example of "a sequence diagram". I'm also curious how the model acts on instructions such as "may not need".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is mostly like "if it decides that are multiple components", but IMO we can always just ask the model later "btw, generate a mermaid diagram for me" and it does a really decent job :)
|
/lgtm |
|
/hold cancel |
This change adds an intiial "add-enhancement" to be used by Claude. The idea is to reduce the time of bootstrapping a new enhancement proposal, by using the current template and some well known questions and fill as much as possible the EP template for the caller.
Example of a generated proposal: #1871