-
Notifications
You must be signed in to change notification settings - Fork 4.5k
UI: hackweek: add copilot instructions to ui repo #31361
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
Conversation
|
CI Results: |
2345256 to
0f57c38
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
0f57c38 to
821ac86
Compare
821ac86 to
b36b1b7
Compare
.github/copilot-instructions.md
Outdated
| @@ -0,0 +1,265 @@ | |||
| # GitHub Copilot Instructions for Vault | |||
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.
👋 Great to see the adoption of instructions. I spent the full HackWeek on investigating instructions and best practice. You might like to read https://go.hashi.co/rfc/eng-051.
One way to improve this setup is to:
- split this top level instructions up into smaller, targeted instruction files each of the files has a glob pattern defined. In VS Code the files would then automatically be included in every prompt in the chat mode. In this case it seems one could split us as follows:
- js.instructions.md (general js best practice)
- ember.instructions.md (ember specific rules)
- api.instructions.md (Restful)
- Follow general prompt best practice:
- short, concise bullet points
- Prefer positive over negative instructions
You can find more examples and inspiration in the doc above.
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.
Finally, we have created a shared repo for instructions within HashiCorp https://github.com/hashicorp/copilot-resources
6bf75a9 to
5f9a7c0
Compare
|
Build Results: |
5f9a7c0 to
fb05635
Compare
hellobontempo
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.
Left some comments, nice work!
56fcdd9 to
38e97b0
Compare
| }); | ||
| ``` | ||
|
|
||
| > **Note**: Avoid manual server setup and shutdown. Use `setupMirage(hooks)` instead to prevent memory leaks and reduce boilerplate code. |
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.
👏 😍 😍 😍
| }); | ||
| ``` | ||
|
|
||
| **Avoid** - unnecessary nesting for organization only: |
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.
Love! great work updating these
hellobontempo
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.
🚀
ryancragun
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.
Two caveats here:
- I don't use VSCode and as such have no idea what the actual outcomes of my suggestions will be for those who do. This PR has motivated me to set aside some time to test copilot in
nvimand see what happens. - I did not really review the ember sections as I'm by no means any sort of authority in that area of the code base.
That said, I'm 👍 on merging this with a few small tweaks to the golang and testing sections.
We might want to hold off on merging this until next week as it'll be easier to deal with the backports that touch .github, but that's entirely up to you. If you do merge it I suspect you'll have to manually backport the changes to vault-enterprise release branches for 1.16.x+ent, 1.18.x+ent, and 1.19.x+ent.
| # Go Programming Instructions | ||
|
|
||
| ## Code Style and Formatting | ||
| - Use `gofmt` and `goimports` to format all Go code automatically |
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.
We use gofumpt, gosimports, and golines formatting
| ## Control Structures | ||
| - Use initialization statement in if/switch when appropriate: | ||
| ```go | ||
| if err := file.Chmod(0664); err != nil { |
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.
| if err := file.Chmod(0664); err != nil { | |
| if err := file.Chmod(0o664); err != nil { |
| ## Interfaces and Composition | ||
| - Design small, focused interfaces (often single-method) | ||
| - Accept interfaces, return concrete types | ||
| - Use empty interface `interface{}` or any sparingly |
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.
| - Use empty interface `interface{}` or any sparingly | |
| - Use empty interface `interface{}` or `any` sparingly |
| ## General Guidelines | ||
| - Write unit tests for all public functions | ||
| - Use table-driven tests when appropriate | ||
| - Follow naming convention: `TestFunctionName` |
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.
We often actually underscore in Vault. e.g. Test_FunctionName
|
|
||
| ## Test Structure | ||
| - Use t.Helper() to mark helper functions | ||
| - Use subtests for organizing related test cases with t.Run() |
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 would also suggest using t.Parallel(). Not only does this yield faster test run, it also encourages writing tests that do not share state or require specific ordering.
| - Use testcontainers to spin up dependencies like postgres or redis | ||
|
|
||
| ## Black box testing | ||
| - The test should be in feature_test package to be the first client of the feature package |
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.
Eventually we'll want to add a section here on end-to-end testing with enos and cloud first testing in Vault Cloud. That does not need to happen in this PR but I wanted to comment on it for posterity.
| - A unit test should only fail for one reason | ||
| - Do not share state between tests | ||
| - Cleanup resources after the test | ||
| - If files are needed, use a temp directory |
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 mention using t.TempDir() as it can create one for you that is automatically cleaned up.
use copilot resources example repo (https://github.com/hashicorp/copilot-resources/tree/main)
38e97b0 to
acecb74
Compare
|
Addressed PR comments & cherrypicked changes to |
…ashicorp#8728) * add copilot instructions to repository use copilot resources example repo (https://github.com/hashicorp/copilot-resources/tree/main) * address PR comments (hashicorp#31361) Co-authored-by: Shannon Roberts (Beagin) <[email protected]>
Description
What does this PR do?
Hackweek: add copilot instructions to ui repo