Skip to content
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

[nodejs@noop_proxy_name] WIP Try to get TARGET_BRANCH from PR's title #3675

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

iunanua
Copy link
Contributor

@iunanua iunanua commented Dec 13, 2024

Motivation

Allow testing of tracer branches that are under development by specifying the branch in the PR's title with [target_language@branch_name_to_test]

Changes

  • New get_target_branch github action to extract branch from PR's title
  • Modify load-binary.sh to read $TARGET_BRANCH env var and use it instead the hardcoded "master" branch.
  • Modify system_tests job name to include target-branch if any
  • Add new fail-if-target-branch job which fails if PR title contains a target branch to avoid merging ST PRs without testing the real main/master branch

Notes

It is working for cpp, agent, nodejs, ruby and python

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • If PR title starts with [<language>], double-check that only <language> is impacted by the change
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

@iunanua iunanua changed the title [nodejs] WIP Try to get TARGET_BRANCH from PR body [nodejs] WIP Try to get TARGET_BRANCH from PR's body Dec 13, 2024
@iunanua iunanua changed the title [nodejs] WIP Try to get TARGET_BRANCH from PR's body [nodejs] [branch:new_user_collection] WIP Try to get TARGET_BRANCH from PR's title Dec 13, 2024
@iunanua iunanua changed the title [nodejs] [branch:new_user_collection] WIP Try to get TARGET_BRANCH from PR's title [nodejs] WIP Try to get TARGET_BRANCH from PR's title Dec 16, 2024
@iunanua iunanua changed the title [nodejs] WIP Try to get TARGET_BRANCH from PR's title [nodejs] [branch:not_existing_branch] WIP Try to get TARGET_BRANCH from PR's title Dec 16, 2024
@iunanua iunanua changed the title [nodejs] [branch:not_existing_branch] WIP Try to get TARGET_BRANCH from PR's title [nodejs] [branch:new_user_collection] WIP Try to get TARGET_BRANCH from PR's title Dec 16, 2024
@iunanua iunanua changed the title [nodejs] [branch:new_user_collection] WIP Try to get TARGET_BRANCH from PR's title [nodejs] WIP Try to get TARGET_BRANCH from PR's title Dec 16, 2024
@iunanua iunanua changed the title [nodejs] WIP Try to get TARGET_BRANCH from PR's title [nodejs] [branch:new_user_collection] WIP Try to get TARGET_BRANCH from PR's title Dec 16, 2024
@iunanua iunanua changed the title [nodejs] [branch:new_user_collection] WIP Try to get TARGET_BRANCH from PR's title [python] [branch:gnufede/sca-standalone] WIP Try to get TARGET_BRANCH from PR's title Dec 16, 2024
@iunanua iunanua changed the title [python] [branch:gnufede/sca-standalone] WIP Try to get TARGET_BRANCH from PR's title [ruby] [branch:vpellan/move-dd-apm-enabled-tag] WIP Try to get TARGET_BRANCH from PR's title Dec 16, 2024
Copy link
Member

@p-datadog p-datadog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a handy feature to me 👍

.github/actions/get_target_branch/action.yaml Outdated Show resolved Hide resolved
@iunanua iunanua changed the title [ruby] [branch:vpellan/move-dd-apm-enabled-tag] WIP Try to get TARGET_BRANCH from PR's title [nodejs] [branch:ugaitz/fixing-ldapjs-flaky] WIP Try to get TARGET_BRANCH from PR's title Dec 17, 2024
@iunanua iunanua changed the title [nodejs] WIP Try to get TARGET_BRANCH from PR's title [nodejs@ugaitz/fix-esm-instrumentations] WIP Try to get TARGET_BRANCH from PR's title Dec 19, 2024
@iunanua iunanua changed the title [nodejs@ugaitz/fix-esm-instrumentations] WIP Try to get TARGET_BRANCH from PR's title [nodejs] WIP Try to get TARGET_BRANCH from PR's title Dec 20, 2024
@iunanua iunanua changed the title [nodejs] WIP Try to get TARGET_BRANCH from PR's title WIP Try to get TARGET_BRANCH from PR's title Dec 20, 2024
@iunanua iunanua marked this pull request as ready for review December 20, 2024 10:24
id: extract
shell: bash
run: |
branch=$(echo "${{ inputs.text }}" | grep -ioP '(?<=\[java|dotnet|python|ruby|php|golang|cpp|agent|nodejs)@.+?\]' | tr -d '[:space:]' || true)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we limit this to supported lang, and explicitly fail if it's not in the supported list?

I'm 100% someone will try to use one of them, and will struggle to understand why it's not working.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, let me try

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added assert_target_branch_is_not_set to force fail if the unsupported libs try to specify a target branch

name: Fail if target branch is specified
needs:
- get_dev_artifacts
if: needs.get_dev_artifacts.outputs.target-branch != ''
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you tested ? I don;t understand where the output of the get_dev_artifacts job is set 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been modified to output the steps.get-target-branch.outputs.target-branch value (lines 66 and 67)

@iunanua iunanua changed the title WIP Try to get TARGET_BRANCH from PR's title [java@this_should_fail] WIP Try to get TARGET_BRANCH from PR's title Jan 3, 2025
@iunanua iunanua changed the title [java@this_should_fail] WIP Try to get TARGET_BRANCH from PR's title [nodejs@noop_proxy_name WIP Try to get TARGET_BRANCH from PR's title Jan 7, 2025
@iunanua iunanua changed the title [nodejs@noop_proxy_name WIP Try to get TARGET_BRANCH from PR's title [nodejs@noop_proxy_name] WIP Try to get TARGET_BRANCH from PR's title Jan 7, 2025
@iunanua iunanua changed the title [nodejs@noop_proxy_name] WIP Try to get TARGET_BRANCH from PR's title [java] WIP Try to get TARGET_BRANCH from PR's title Jan 7, 2025
@iunanua iunanua changed the title [java] WIP Try to get TARGET_BRANCH from PR's title [nodejs@noop_proxy_name] WIP Try to get TARGET_BRANCH from PR's title Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants