First take at documented coding standards #1429
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR establishes documented coding standards for the terraform-provider-elasticstack repository to improve AI-assisted code review quality. The standards document provides clear guidelines on project structure, schema definitions, JSON handling, resource implementation, testing, and API client usage, while the updated Copilot instructions reference these standards to ensure consistent enforcement during both code writing and review activities.
Key Changes:
- Added comprehensive CODING_STANDARDS.md document covering Go conventions, project structure, and Terraform provider best practices
- Updated Copilot instructions to reference and enforce the new coding standards
- Refined instructions to distinguish between code writing and code review contexts
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| CODING_STANDARDS.md | New document establishing project-wide coding conventions and best practices for Plugin Framework resources, schema definitions, JSON handling, testing, and API client usage |
| .github/copilot-instructions.md | Updated to reference CODING_STANDARDS.md and clarify instructions for both code writing and code review scenarios |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
nick-benoit
left a comment
nick-benoit
left a comment
There was a problem hiding this comment.
Nice, i'm excited to see how this works. Thanks for pushing this forward.
| ## Project Structure | ||
|
|
||
| - Use the Plugin Framework for all new resources (not SDKv2) | ||
| - Follow the code organization pattern of `internal/elasticsearch/security/system_user` for new Plugin Framework resources |
There was a problem hiding this comment.
I wonder if it's worth listing all the files in that directory and explaining what they are for to make it more straightforward for copilot to understand the structure? Alternatively adding a file like this to the directory and listing that here could be useful. This is probably not necessary for all resources, but I think could be good for our "canonical" resource.
It seems like giving copilot the summary might yield more consistent performance than having copilot parse the files and comprehend how they all fit together in one go.
| - Long, multiline descriptions should be stored in an external markdown file, which is imported via Golang embedding. See `internal/elasticsearch/security/system_user/resource-description.md` for an example location. | ||
| - Use schema validation wherever possible. Only perform validation within create/read/update functions as a last resort. | ||
| - For example, any validation that relies on the actual Elastic Stack components (e.g Elasticsearch version) | ||
| can only be performed during the create/read/update phase. |
There was a problem hiding this comment.
I wonder if we should be explicitly asking co-pilot to generate the OAS and consider this when generating schema definitions?
There was a problem hiding this comment.
I guess we could also consider committing this file? I suppose currently we are ignoring the client oas file. I'm sure this would lead to some annoying merge conflicts etc, but maybe is worth it in order to make it easier for copilot to consider the oas?
|
|
||
| ## Resource Implementation | ||
|
|
||
| - Follow the pattern: `resource.go`, `schema.go`, `models.go`, `create.go`, `read.go`, `update.go`, `delete.go` |
There was a problem hiding this comment.
This seems like it maybe overlaps with ## Project Structure. I wonder if we should combine both of those here? Or move higher level details to "Project Structure" and reserve this section for more fiddly details like "utils.IsKnown(val) instead of !val.IsNull() && !val.IsUnknown()"
I don't feel strongly about this, just a thought.
The intention here is to try and increase the value we get out of Copilot code review. Overall, the aim is to allow human reviewers to focus attention on higher level concerns, with confidence the AI reviews are enforcing more formulaic coding style/expectations, kind of lint on steroids.