-
Notifications
You must be signed in to change notification settings - Fork 121
Extend CONTRIBUTING.md #1262
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
Extend CONTRIBUTING.md #1262
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,60 +1,145 @@ | ||||||||
| # Typical development workflow | ||||||||
| # Contributing | ||||||||
|
|
||||||||
| Fork the repo, work on an issue | ||||||||
| This guide explains how to set up your environment, make changes, and submit a PR. | ||||||||
|
|
||||||||
| ## Updating the generated Kibana client. | ||||||||
| ## Development Setup | ||||||||
|
|
||||||||
| If your work involves the Kibana API, the endpoints may or may not be included in the generated client. | ||||||||
| Check [generated/kbapi](./generated/kbapi/) for more details. | ||||||||
| * Fork and clone the repo. | ||||||||
| * Setup your preferred IDE (Intelliji, VSCode, etc.) | ||||||||
|
|
||||||||
| ## Acceptance tests | ||||||||
| Requirements: | ||||||||
| * [Terraform](https://www.terraform.io/downloads.html) >= 1.0.0 | ||||||||
| * [Go](https://golang.org/doc/install) >= 1.25 | ||||||||
| * Docker (for acceptance tests) | ||||||||
|
|
||||||||
| ```bash | ||||||||
| make docker-testacc | ||||||||
| ``` | ||||||||
| ## Development Workflow | ||||||||
|
|
||||||||
| Run a single test with terraform debug enabled: | ||||||||
| ```bash | ||||||||
| env TF_LOG=DEBUG make docker-testacc TESTARGS='-run ^TestAccResourceDataStreamLifecycle$$' | ||||||||
| ``` | ||||||||
| * Create a new branch for your changes. | ||||||||
| * Make your changes. See [Useful Commands](#useful-commands) and [Debugging](#debugging). | ||||||||
| * GitHub Copilot can be used to help with these changes (See [Using Copilot](#using-copilot)) | ||||||||
| * Validate your changes | ||||||||
| * Run unit and acceptance tests (See [Running Acceptance Tests](#running-acceptance-tests)). | ||||||||
| * Run `make fmt`, `make lint`. | ||||||||
| * All checks also run automatically on every PR. | ||||||||
| * Add a changelog entry in `CHANGELOG.md` under the `Unreleased` section. This will be included in the release notes of the next release. | ||||||||
| * Submit your PR for review. | ||||||||
|
|
||||||||
| A way to forward debug logs to a file: | ||||||||
| ```bash | ||||||||
| env TF_ACC_LOG_PATH=/tmp/tf.log TF_ACC_LOG=DEBUG TF_LOG=DEBUG make docker-testacc | ||||||||
| ``` | ||||||||
| When creating new resources: | ||||||||
| * Use the [Plugin Framework](https://developer.hashicorp.com/terraform/plugin/framework/getting-started/code-walkthrough) for new resources. | ||||||||
| * Use an existing resource (e.g. `internal/elasticsearch/security/system_user`) as a template. | ||||||||
gigerdo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| * Some resources use the deprecated Terraform SDK, so only resources using the new Terraform Framework should be used as reference. | ||||||||
| * Use the generated API clients to interact with the Kibana APIs. (See [Working with Generated API Clients](#working-with-generated-api-clients) | ||||||||
| * Add documentation and examples for the resource. Update the generated docs with `make docs-generate`. | ||||||||
|
||||||||
| * Write unit and acceptance tests. | ||||||||
|
|
||||||||
|
|
||||||||
| ## Update documentation | ||||||||
| ## Using Copilot | ||||||||
|
|
||||||||
| Update documentation templates in `./templates` directory and re-generate docs via: | ||||||||
| ```bash | ||||||||
| make docs-generate | ||||||||
| ``` | ||||||||
| GitHub Copilot can speed up development, but you’re responsible for correctness: | ||||||||
| * Create an issue describing the desired change and acceptance criteria. | ||||||||
| * Assign the issue to Copilot and iterate with prompts. | ||||||||
| * Review outputs carefully; add tests and adjust as needed. | ||||||||
| * Example issue: https://github.com/elastic/terraform-provider-elasticstack/issues/1219 | ||||||||
|
||||||||
|
|
||||||||
| ## Update `./CHANGELOG.md` | ||||||||
| ### Useful Commands | ||||||||
|
|
||||||||
| List of previous commits is a good example of what should be included in the changelog. | ||||||||
| * `make build`: Build the provider. | ||||||||
| * `make install`: Install the provider. | ||||||||
|
||||||||
| * `make fmt`: Format the code. | ||||||||
| * `make lint`: Lint the code. | ||||||||
|
||||||||
| * `make fmt`: Format the code. | |
| * `make lint`: Lint the code. | |
| * `make lint`: Lint the code. |
Lint also formats
gigerdo marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
This path is can be quite slow, it both spins up the Elastic Stack docker containers (Yay!), but also runs the actual provider tests inside a golang docker container.
Repeated testing (i.e what we'd expect in a dev workflow) is much faster if we re-use a single running Stack environment but also run the tests directly on the dev machine.
| make docker-testacc | |
| make docker-fleet | |
| make testacc |
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.
Sounds good, I updated the docs to use this faster workflow.
Outdated
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.
| - `internal/clients/*`: Manually written clients. These should only be used/extended if it is not possible to use the generated clients. |
This isn't really true. Much of this package configures one of the Stack clients, providing endpoints/credentials/etc from the provider configuration to the client.
There's some code in here which is used as a bridge between the provider resources and the older Kibana/Elasticsearch clients. We don't need to continue with that as much, but there are still cases where it may make sense, for instance in Kibana connectors have a bunch of non-trivial logic managing API defaults which kind of makes sense in a client layer.
I don't think there's a need for a hard rule on not making additions 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.
True, I didn't realize this also contains all the wrapper code for the generated client. I'll remove that entry.
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.
Currently changelog entries link back to the PR introducing the change which means we first need to open the PR and then add the changelog entry. That situation has never really made sense, but it does make sense to be consistent for now.
IMO rather than improving the ordering here, it would likely make more sense to look at automatically adding changelog entries based on the PR title.
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'll reorder it for now. Though it would be nice to have this automated so the release job just collects release notes from all PRs included in the release.