-
Notifications
You must be signed in to change notification settings - Fork 194
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
Improve Contributing guide #1501
base: main
Are you sure you want to change the base?
Changes from all commits
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,3 +1,170 @@ | ||
# Contributing to OpenTelemetry PHP | ||
|
||
## Introduction | ||
|
||
Welcome to the OpenTelemetry PHP repository! 🎉 | ||
|
||
Thank you for considering contributing to this project. Whether you're fixing a bug, adding new features, improving documentation, or reporting an issue, we appreciate your help in making OpenTelemetry better. | ||
|
||
This repository is part of the OpenTelemetry ecosystem, which provides observability tooling for distributed systems. Your contributions help enhance the PHP ecosystem for OpenTelemetry users worldwide. | ||
|
||
If you have any questions, feel free to ask in the community channels. We’re happy to help! 😊 | ||
|
||
## Pre-requisites | ||
|
||
Before getting started, ensure you have the following installed: | ||
|
||
- **PHP** (8.1 or higher) – [Install PHP](https://www.php.net/downloads) | ||
- **Composer** – [Install Composer](https://getcomposer.org/) | ||
- **Docker & Docker Compose** – [Install Docker](https://docs.docker.com/engine/install/) | ||
- **Make** (for running development tasks) | ||
|
||
Additional Notes: | ||
- Windows users may need [Git Bash](https://gitforwindows.org/) for better compatibility. | ||
- Consider using [phpenv](https://github.com/phpenv/phpenv) for managing multiple PHP versions. | ||
|
||
## Workflow | ||
|
||
We follow a structured workflow to ensure smooth collaboration: | ||
|
||
### Branch Naming Convention | ||
- **Feature branches**: `feature/<short-description>` | ||
- **Bugfix branches**: `fix/<short-description>` | ||
- **Documentation updates**: `docs/<short-description>` | ||
|
||
### Commit Message Format | ||
- Use descriptive commit messages (e.g., `fix(tracing): resolve issue with span context`) | ||
- Follow [Conventional Commits](https://www.conventionalcommits.org/) where possible. | ||
|
||
### Pull Request Guidelines | ||
- Fork the repository and create a new branch. | ||
- Follow the coding guidelines before submitting your PR. | ||
- Ensure tests pass locally before pushing. | ||
- Link relevant issues in the PR description. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does each PR require an issue? I have a trivial PR and will make an issue for it, but it'd be good if there were guidance in case there are some circumstances one isn't required There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a contributor myself, so I guess the maintainers would have better knowledge to give a response to this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy to receive PRs without a corresponding issue, provided the PR provides some detail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm the same as @brettmc. Although there's a risk that contributors spend more time on a PR that would not have been necessary with some up-front discussion in an issue/other channel. |
||
|
||
|
||
## Local Run/Build | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi I did my first work on this repo a few days ago, and the makefile doesn't work straight after a clone because of this line
I see what it does now, but some guidance on how to provision one would have been helpful There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How to set up There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh bloody hell. I did look for something like |
||
|
||
To set up your local development environment: | ||
|
||
```bash | ||
# Clone the repository | ||
git clone https://github.com/open-telemetry/opentelemetry-php.git | ||
cd opentelemetry-php | ||
|
||
# Install dependencies | ||
make install | ||
``` | ||
|
||
To update dependencies: | ||
|
||
```bash | ||
make update | ||
``` | ||
|
||
To run checks: | ||
|
||
```bash | ||
make all-checks | ||
``` | ||
|
||
|
||
## Testing | ||
|
||
To ensure your changes meet our project's standards, simply run: | ||
|
||
```bash | ||
RichardChukwu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
make all | ||
|
||
``` | ||
|
||
To run tests against different PHP versions: | ||
|
||
```bash | ||
PHP_VERSION=8.3 make test | ||
``` | ||
|
||
## Contributing Rules | ||
|
||
- Follow [Clean Code PHP](https://github.com/jupeter/clean-code-php) principles. | ||
- Ensure new features have appropriate test coverage. | ||
- Run `make style` before submitting a PR. | ||
- Include clear and concise documentation updates if needed. | ||
|
||
Check for issues labeled [`good first issue`](https://github.com/open-telemetry/opentelemetry-php/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22) to start contributing. | ||
|
||
|
||
## Further Help | ||
|
||
Need help? Join our community: | ||
|
||
Most of our communication is done on CNCF Slack in the channel: | ||
|
||
- **Slack**: [otel-php](https://cloud-native.slack.com/archives/C01NFPCV44V) | ||
- **GitHub Discussions**: [OpenTelemetry PHP Discussions](https://github.com/open-telemetry/opentelemetry-php/discussions) | ||
- **Issues**: If you encounter a bug, [open an issue](https://github.com/open-telemetry/opentelemetry-php/issues) | ||
|
||
|
||
## Troubleshooting Guide | ||
|
||
### Common Issues & Fixes | ||
|
||
#### 1. Missing PHP dependencies | ||
**Error:** `Class 'SomeClass' not found` | ||
|
||
**Fix:** Run: | ||
```bash | ||
make install | ||
``` | ||
|
||
#### 2. Linting Errors | ||
**Error:** `Files not formatted correctly` | ||
|
||
**Fix:** Run: | ||
```bash | ||
make style | ||
``` | ||
|
||
#### 3. Tests Failing Due to Missing Dependencies | ||
**Error:** `Dependency missing` | ||
|
||
**Fix:** | ||
```bash | ||
make update | ||
make test | ||
``` | ||
|
||
|
||
## Additional Information | ||
|
||
### Code Coverage | ||
We use [Codecov](https://about.codecov.io/) to track test coverage. You can generate a local coverage report using: | ||
|
||
```bash | ||
make test-coverage | ||
``` | ||
|
||
### Generating API Documentation | ||
To generate API docs: | ||
```bash | ||
make phpdoc | ||
``` | ||
|
||
To preview locally: | ||
```bash | ||
make phpdoc-preview | ||
``` | ||
|
||
### Dependency Validation | ||
We use [Deptrac](https://github.com/qossmic/deptrac) for dependency validation: | ||
```bash | ||
make deptrac | ||
``` | ||
|
||
Thank you for contributing! 🚀 | ||
|
||
|
||
|
||
## Maintainers | ||
[@open-telemetry/php-maintainers](https://github.com/orgs/open-telemetry/teams/php-maintainers) | ||
|
||
|
@@ -34,10 +201,6 @@ Find more information about the member role in the [community repository](https: | |
|
||
Find more about emeritus roles in the [community repository](https://github.com/open-telemetry/community/blob/main/community-membership.md#emeritus-maintainerapprovertriager) | ||
|
||
## Communication | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for this to not be mentioned? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is mentioned actually in the guide, see under "Further Help" and the last information in the doc. @brettmc |
||
|
||
Most of our communication is done on CNCF Slack in the channel [otel-php](https://cloud-native.slack.com/archives/C01NFPCV44V). | ||
To sign up, create a CNCF Slack account [here](http://slack.cncf.io/) | ||
|
||
Our meetings are held weekly on zoom on Wednesdays at 10:30am PST / 1:30pm EST. | ||
A Google calendar invite with the included zoom link can be found [here](https://calendar.google.com/event?action=TEMPLATE&tmeid=N2VtZXZmYnVmbzZkYjZkbTYxdjZvYTdxN21fMjAyMDA5MTZUMTczMDAwWiBrYXJlbnlyeHVAbQ&tmsrc=google.com_b79e3e90j7bbsa2n2p5an5lf60%40group.calendar.google.com&scp=ALL) | ||
|
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 don't currently have a standard or practise for branch naming or commit messages. I assume this has come from another repository? Is it considered best-practise for all OpenTelemetry projects?
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.
Not sure if there any general format though, it can be removed if there is no standard yet for this repo though, or can be adopted this depending on what is tenable