Skip to content

Commit 794ff14

Browse files
PerfectSlayerPerfectSlayer
authored
Improve onboarding and testing documentation, add stale PR cleanup automation, and fix milestone automation (#7663)
* feat: Add PR cleanup automation * feat: Update pull request documentation from contribution guidelines * feat: Improve setup script * feat: Adding disambiguation about running tests locally * feat: Add git submodules initialization check * feat: Improve testing documentation * fix: Fix milestone automation
1 parent 6cd03d2 commit 794ff14

8 files changed

Lines changed: 115 additions & 24 deletions

File tree

‎.github/workflows/README.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,15 @@ _Action:_ Clean up old lib-injection OCI images from GitHub Container Registry.
8282

8383
_Recovery:_ Manually trigger the action again.
8484

85+
### prune-old-pull-requests [🔗](prune-old-pull-requests.yaml)
86+
87+
_Trigger:_ Every month or manually.
88+
89+
_Action:_ Mark as stale and comment on pull requests with no update during the last quarter.
90+
Close them if no following update within a week.
91+
92+
_Recovery:_ Manually trigger the action again.
93+
8594
## Code Quality and Security
8695

8796
### analyze-changes [🔗](analyze-changes-with-github-codeql.yaml)

‎.github/workflows/increment-milestone-on-tag.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ jobs:
5555
const nextMilestoneTitle = versionNumbers.join('.')
5656
core.info(`Creating next version milestone: ${nextMilestoneTitle}`)
5757
// Create the next milestone
58-
await github.issues.createMilestone({
58+
await github.rest.issues.createMilestone({
5959
owner: context.repo.owner,
6060
repo: context.repo.repo,
6161
title: nextMilestoneTitle

‎.github/workflows/prune-old-pull-requests.yaml‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
name: Prune old pull requests
2+
on:
3+
schedule:
4+
- cron: '0 3 1 * *'
5+
workflow_dispatch:
6+
7+
jobs:
8+
prune-old-pull-requests:
9+
name: Prune old pull requests
10+
runs-on: ubuntu-latest
11+
permissions:
12+
issues: write
13+
pull-requests: write
14+
steps:
15+
- name: Prune old pull requests
16+
uses: actions/stale@28ca1036281a5e5922ead5184a1bbf96e5fc984e # v9.0.0
17+
with:
18+
days-before-stale: -1 # Disable general stale bot
19+
days-before-pr-stale: 90 # Only enable stale bot for PRs with no activity for 90 days
20+
stale-pr-message: 'This pull request has been marked as stale because it has not had activity over the past quarter. It will be closed in 7 days if no further activity occurs. Feel free to reopen the PR if you are still working on it.'
21+
close-pr-message: 'This pull request has been closed because it has not had activity over the past quarter. Feel free to reopen the PR if you are still working on it.'
22+
stale-pr-label: 'tag: stale'
23+
operations-per-run: 500

‎BUILDING.md‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ $ ./setup.sh
2525
✅ The git command line is installed.
2626
✅ pre-commit hook is installed in repository.
2727
✅ git config submodule.recurse is set to true.
28+
✅ All git submodules are initialized.
2829
ℹ️ Checking Docker environment:
2930
✅ The docker command line is installed.
3031
✅ The Docker server is running.
@@ -190,4 +191,14 @@ To build the entire project with tests (this can take a very long time) run:
190191
./gradlew clean build
191192
```
192193

193-
After building the project, you can find the jar build artifact into the `dd-java-agent/build/libs` folder.
194+
>[!NOTE]
195+
> Running the complete test suite on a local development environment can be challenging.
196+
> It might take a very long time, and you might encounter few flaky tests along the way.
197+
> It is recommended to only run the tests related to your changes locally, and leave the whole test suite to the continuous integration platform.
198+
199+
To build the JVM agent artifact only run:
200+
```bash
201+
./gradlew :dd-java-agent:shadowJar
202+
```
203+
204+
After building the project, you can find the built JVM agent artifact into the `dd-java-agent/build/libs` folder.

‎CONTRIBUTING.md‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,8 @@
44

55
Pull requests for bug fixes are welcome, but before submitting new features or changes to current
66
functionality, please [open an issue](https://github.com/DataDog/dd-trace-java/issues/new)
7-
and discuss your ideas or propose the changes you wish to make first. After a resolution is reached a PR can be
8-
submitted for
9-
review.
10-
11-
When opening a pull request, please open it as
12-
a [draft](https://github.blog/2019-02-14-introducing-draft-pull-requests/) to not auto assign reviewers before you feel
13-
the pull request is in a reviewable state.
7+
and discuss your ideas or propose the changes you wish to make first. After a resolution is reached a [PR can be
8+
submitted](#pull-request-guidelines) for review.
149

1510
## Adding instrumentations
1611

@@ -91,6 +86,10 @@ command line should be Java 8.
9186

9287
## Pull Request Guidelines
9388

89+
### Draft first
90+
91+
When opening a pull request, please open it as a [draft](https://github.blog/2019-02-14-introducing-draft-pull-requests/) to not auto assign reviewers before you feel the pull request is in a reviewable state.
92+
9493
### Title Format
9594

9695
Pull request titles should briefly describe the proposed changes in a way that makes sense for the users.
@@ -133,3 +132,17 @@ Labels are not only used to categorize but also alter the continuous integration
133132
>[!NOTE]
134133
> For reference, the [full list of all labels available](https://github.com/DataDog/dd-trace-java/labels).
135134
> If you feel one is missing, let [the maintainer team](https://github.com/orgs/DataDog/teams/apm-java) know!
135+
136+
## Pull Request Reviews
137+
138+
### Review Expectations
139+
140+
After making you pull request ready for review by converting it from draft, you can expect getting an initial review comment within two working days, and a full review within a week of work.
141+
If you don't receive any update, feel free to send a nice reminder to the assigned reviewers using pull request comments or our internal Slack channel.
142+
143+
### Stale Pull Requests
144+
145+
A pull request is considered "stale" if it has had no activity (comments, updates) for the last quarter.
146+
Stale PRs will be commented and labelled as such (using the `tag: stale` label), then closed if they still receive no update for a week after.
147+
148+
Closed PRs can be reopened at any time, but may be closed again if they ever meet the same stale conditions.

‎docs/how_to_test.md‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ Spock framework provides an alternative for more complex test scenarios, or test
1212
2. A variant of unit tests are **instrumented tests**.
1313
Their purpose is similar to the unit tests but the tested code is instrumented by the java agent (`:dd-trace-java:java-agent`) while running. They extend the Spock specification `datadog.trace.agent.test.AgentTestRunner` which allows to test produced traces and metrics.
1414

15-
3. The third type of tests are **Muzzle checks.
15+
3. The third type of tests are **Muzzle checks**.
1616
Their goal is to check the [Muzzle directives](./how_instrumentations_work.md#muzzle), making sure instrumentations are safe to load against specific library versions.
1717

1818
3. The fourth type of tests are **integration tests**.
@@ -31,22 +31,32 @@ They are intended to test behavior consistency between all the client libraries,
3131
### Forked Tests
3232

3333
Independently of the type of test, test can be run in another (forked) JVM than the one running Gradle.
34-
This behavior is implicit when the test class name is suffixed by `ForkedTest` (eg `SomeFeatureForkedTest`). This mechanism exists to make sure either java agent state or static data are reset between test runs.
34+
This behavior is implicit when the test class name is suffixed by `ForkedTest` (eg `SomeFeatureForkedTest`).
35+
This mechanism exists to make sure either java agent state or static data are reset between test runs.
36+
37+
> [!NOTE]
38+
> Forked tests are not run part of the gradle `test` task.
39+
> In order to run them, you need to use the `forkedTest` task instead.
3540
3641
### Flaky Tests
3742

3843
If a test runs unreliably, or doen't have a fully deterministic behavior, this will lead into recurrent unexpected errors in continuous integration.
3944
In order to identify such tests and avoid the continuous integration to fail, they are marked as _flaky_ and must be annotated with the `@Flaky` annotation.
4045

46+
> [!TIP]
47+
> In case your pull request checks failed due to some unexpected flaky tests, you can retry the continous integration pilepeline on CircleCI using the `Rerun workflow from failed` button:
48+
49+
![Rerun workflow from failed](how_to_test/rerun-workflow-from-failed.png)
50+
4151
## Running Tests
4252

4353
You can run the whole project test suite using `./gradlew test` but expect it to take a certain time.
44-
Instead, you can run test for a specific module (ex. `:dd-java-agent:instrumentation:opentelemetry:opentelemetry-1.4`) using the test command for this module only: `./gradlew :dd-java-agent:instrumentation:opentelemetry:opentelemetry-1.4:test`.
54+
Instead, you can run tests for a specific module (ex. `:dd-java-agent:instrumentation:opentelemetry:opentelemetry-1.4`) using the test command for this module only: `./gradlew :dd-java-agent:instrumentation:opentelemetry:opentelemetry-1.4:test`.
4555

4656
> [!TIP]
4757
> Flaky tests can be disabled by setting the Gradle property `skipFlakyTests` (ex. `./gradlew -PskipFlakyTests <task>`).
4858
49-
### Running tests on another JVM
59+
### Running Tests on Another JVM
5060

5161
To run tests on a different JVM than the one used for doing the build, you need two things:
5262

@@ -59,6 +69,18 @@ To run tests on a different JVM than the one used for doing the build, you need
5969
> [!NOTE]
6070
> Please note that the JDK name needs to end with the JDK version, e.g. `11`, `ZULU15`, `ORACLE8`, `GRAALVM17`, etc.
6171
72+
### Running System Tests
73+
74+
The system tests are setup to run on continous integration as pull request check.
75+
76+
If you would like to run them locally, you would have to grab [a local copy of the system tests](https://github.com/DataDog/system-tests), and run them from there.
77+
You can make them use your development version of `dd-trace-java` by [dropping the built artifacts to the `/binaries` folder](https://github.com/DataDog/system-tests/blob/main/docs/execute/binaries.md#java-library) of your local copy of the system tests.
78+
79+
If you would like to run another version of the system tests on continuous integration, or update them to the latest version, you would need to use [the update pinned system tests script](../.circleci/update_pinned_system_tests.sh) as your pull request won't use the latest `main` version from the system test repository, but a pinned version.
80+
81+
> [!NOTE]
82+
> The system tests version used for continous integration is defined using `default_system_tests_commit` in [CircleCI configuration](../.circleci/config.continue.yml.j2).
83+
6284
### The APM test agent
6385

6486
The APM test agent emulates the APM endpoints of the Datadog Agent.

‎docs/how_to_test/rerun-workflow-from-failed.png‎

29.9 KB
Loading

‎setup.sh‎

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,14 @@
77
function check-jvm() {
88
local JAVA_HOME_NAME=$1
99
local EXPECTED_JAVA_VERSION=$2
10-
if [ -z ${!JAVA_HOME_NAME} ]; then
10+
if [ -z "${!JAVA_HOME_NAME}" ]; then
1111
echo "❌ $JAVA_HOME_NAME is not set. Please set $JAVA_HOME_NAME to refer to a JDK $EXPECTED_JAVA_VERSION installation." >&2
1212
exit 1
13-
elif ! ${!JAVA_HOME_NAME}/bin/java -version 2>&1 | grep -q "version \"$EXPECTED_JAVA_VERSION" ; then
13+
elif ! "${!JAVA_HOME_NAME}/bin/java" -version 2>&1 | grep -q "version \"$EXPECTED_JAVA_VERSION" ; then
1414
echo "❌ $JAVA_HOME_NAME is set to ${!JAVA_HOME_NAME}, but it does not refer to a JDK $EXPECTED_JAVA_VERSION installation." >&2
1515
exit 1
1616
else
17-
echo "✅ $JAVA_HOME_NAME is set to $(readlink -f ${!JAVA_HOME_NAME})."
17+
echo "✅ $JAVA_HOME_NAME is set to $(readlink -f "${!JAVA_HOME_NAME}")."
1818
fi
1919
}
2020

@@ -35,7 +35,7 @@ check-jvm "JAVA_GRAALVM17_HOME" "17"
3535

3636
function check-command() {
3737
local COMMAND_NAME=$1
38-
if command -v $COMMAND_NAME &> /dev/null; then
38+
if command -v "$COMMAND_NAME" &> /dev/null; then
3939
echo "✅ The $COMMAND_NAME command line is installed."
4040
else
4141
echo "❌ The $COMMAND_NAME command line is missing. Please install $COMMAND_NAME." >&2
@@ -45,14 +45,15 @@ function check-command() {
4545

4646
function get-file-hash() {
4747
local FILE=$1
48-
echo $(md5sum $FILE | awk '{print $1}')
48+
md5sum "$FILE" | awk '{print $1}'
4949
}
5050

5151
function look-for-hook() {
5252
local HOOK_NAME=$1
53-
local HOOK_CHECKSUM=$(get-file-hash .githooks/$HOOK_NAME)
54-
local HOOKS_PATH=$(git config core.hooksPath)
55-
local HOOK_FOUND=false
53+
local HOOK_CHECKSUM
54+
HOOK_CHECKSUM=$(get-file-hash .githooks/$HOOK_NAME)
55+
local HOOKS_PATH
56+
HOOKS_PATH=$(git config core.hooksPath)
5657

5758
if [ -e ".git/hooks/$HOOK_NAME" ] && [ "$(get-file-hash .git/hooks/$HOOK_NAME)" == "$HOOK_CHECKSUM" ]; then
5859
echo "✅ $HOOK_NAME hook is installed in repository."
@@ -66,7 +67,8 @@ function look-for-hook() {
6667
function check-git-config() {
6768
local CONFIG_NAME=$1
6869
local EXPECTED_VALUE=$2
69-
local ACTUAL_VALUE=$(git config $CONFIG_NAME)
70+
local ACTUAL_VALUE
71+
ACTUAL_VALUE=$(git config "$CONFIG_NAME")
7072
if [ "$ACTUAL_VALUE" == "$EXPECTED_VALUE" ]; then
7173
echo "✅ git config $CONFIG_NAME is set to $EXPECTED_VALUE."
7274
elif [ -z "$ACTUAL_VALUE" ]; then
@@ -76,17 +78,27 @@ function check-git-config() {
7678
fi
7779
}
7880

81+
function check-submodule-initialization() {
82+
if [ -e ".gitmodules" ]; then
83+
if git submodule status | grep '^-' > /dev/null; then
84+
echo "❌ A git submodule are not initialized. Please run 'git submodule update --init --recursive'."
85+
else
86+
echo "✅ All git submodules are initialized."
87+
fi
88+
fi
89+
}
90+
7991
echo "ℹ️ Checking git configuration:"
8092
check-command "git"
8193
look-for-hook "pre-commit"
8294
check-git-config "submodule.recurse" "true"
95+
check-submodule-initialization
8396

8497

8598
#
8699
# Check Docker environment.
87100
#
88101

89-
90102
function check-docker-server() {
91103
if docker info &> /dev/null; then
92104
echo "✅ The Docker server is running."
@@ -108,7 +120,8 @@ check-docker-server
108120
function check-ulimit() {
109121
local LIMIT_NAME="File descriptor limit"
110122
local EXPECTED_LIMIT=$1
111-
local ACTUAL_LIMIT=$(ulimit -n)
123+
local ACTUAL_LIMIT
124+
ACTUAL_LIMIT=$(ulimit -n)
112125
if [ "$ACTUAL_LIMIT" -ge "$EXPECTED_LIMIT" ]; then
113126
echo "✅ $LIMIT_NAME is set to $ACTUAL_LIMIT."
114127
else

0 commit comments

Comments
 (0)