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

follow-up work for TAV tests now being in GH Actions #3227

Closed
6 tasks done
trentm opened this issue Mar 29, 2023 · 11 comments
Closed
6 tasks done

follow-up work for TAV tests now being in GH Actions #3227

trentm opened this issue Mar 29, 2023 · 11 comments
Assignees
Labels
8.9-candidate agent-nodejs Make available for APM Agents project planning.
Milestone

Comments

@trentm
Copy link
Member

trentm commented Mar 29, 2023

#3127 moved our TAV tests CI runs from Jenkins to GH Actions. There are a few things to follow-up on with this:

  • I would very much like a way to be able to manually run a subset of the TAV tests on a PR. I can manually run one or more .ci/scripts/test.sh -b "release" -t "${{ module }}" "${{node }}" on my dev machine. However, it is useful to have check results reflected on a PR. Also for community PRs it is nice to run relevant TAV tests and have the results available via the PR. Before, when using Jenkins, we had the run module tests for <modules> GH comment. It doesn't need to be a GitHub comment. See action: tav #3127 (comment) for earlier discussion on this.
  • Are the individual TAV job checks shown for commits on main? For example commit 6660cc3 is the commit that added the new workflow, its workflow did run last night (https://github.com/elastic/apm-agent-nodejs/actions/runs/4552294471), yet the summary of checks for that commit do not include the TAV job runs.
  • Some sort of lint checks on tav.yml would be worthwhile:
    • Is the set of modules up to date? This should match the full set of .tav.yml module/group names for all ".tav.yml" files in the repo (currently there are two).
    • Does number-of-node-versions * number-of-tav-modules exceed the GitHub limit of 256? Currently we are at 245. If we pass the limit, then we'll need to cope somehow. See action: tav #3127 (comment)
  • The "dev-utils/jenkins-build-slow-steps.sh" dev tool should be replaced with one that looks at the GH Action runs. See action: tav #3127 (comment) for a start.

/cc @v1v (Victor, I'm not suggesting you need to do these things. I'll do some of them. Mostly I would appreciate suggestions/advice on the first item.)

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Mar 29, 2023
@v1v
Copy link
Member

v1v commented Mar 29, 2023

Thanks for this, it was something in my TODO list :)

I would very much like a way to be able to manually run a subset of the TAV tests on a PR

Yep, I agree. Let me create a sample github repository with the different alternatives so we can agree which one is the best.

To add more clarity, there are some caveats, for instance, GitHub comments should trigger builds if they are coming from an Elastician user, but IIRC, the existing GITHUB TOKEN created on the fly does not have permissions to access the Elastic org, hence we might need to use some GH PAT secrets and that's not supported when using the on: pull_requests but on: pull_requests_target (https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ explains a bit how we can use it... but the less workflows with secrets the less chances we can get compromised :) )

Are the individual TAV job checks shown for commits on main?

Something is not right with GH actions, likely some of the existing degraded issues that happened today...

See https://github.com/elastic/apm-agent-nodejs/actions/runs/4552294471 is the one for that particular commit

@trentm
Copy link
Member Author

trentm commented Mar 29, 2023

To add more clarity, there are some caveats, for instance, GitHub comments should trigger builds if they are coming from an Elastician user ...

Note that I'm fine if there isn't a way to start this via a GitHub PR comment. For example if it is a separate gh ... command that would be fine to start. E.g. gh workflow run tav.yml --ref trentm/my-feature-branch -f node=16 -f module=fastify

likely some of the existing degraded issues that happened today...

Yah, hopefully. We'll see in coming days.

@v1v
Copy link
Member

v1v commented Mar 30, 2023

https://github.com/elastic-robots/gh-actions-interactions/blob/main/.github/workflows/ci-command.yml is the minimal proposal to use GitHub commands

  1. test github commands elastic-robots/gh-actions-interactions#1 (comment) is the comment to trigger the above with a reaction 👀
  2. test github commands elastic-robots/gh-actions-interactions#1 (comment) is the comment if not supported for that particular user (aka no write/admin permissions in the repo)
  3. test github commands elastic-robots/gh-actions-interactions#1 (comment) is the comment with the reaction 👍 when things worked like a charm

Tasks:

  • Checkout the PR
  • Create GitHub check with the link to the GitHub run
  • Create customisable github commands /test module module1,module2,module3 or /test module module1,module2 in 1.16 or /test module all

What are your thoughts?

@trentm
Copy link
Member Author

trentm commented Mar 30, 2023

https://github.com/elastic-robots/gh-actions-interactions/blob/main/.github/workflows/ci-command.yml is the minimal proposal to use GitHub commands

This looks great.

  1. We have a separate discussion on the trigger condition (test github commands elastic-robots/gh-actions-interactions#1).
  2. Can we use /test tav ... to have the name "tav" in use, rather than module, please?
  3. Also, potentially it would be nice to be able to select a subset of the node versions. /test tav redis,ioredis 19,20. But that can come later.
  4. Next is to figure out how to select from the "tav.yml" matrix with these inputs.

@trentm
Copy link
Member Author

trentm commented Mar 30, 2023

They are showing now.

@v1v
Copy link
Member

v1v commented Apr 5, 2023

#3246 is the one supporting running tav modules individually as discussed in #3227 (comment)

@trentm
Copy link
Member Author

trentm commented Apr 10, 2023

A follow-up on #3232.

An issue with using issue_comment as the workflow trigger is that -- according to https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment and according to my testing -- has the GITHUB_REF associated with the default branch and not associated with the PR branch. The github.event. object does not have a .pull_request object from which to pull a .head.sha of the latest current commit to that branch. An example issue_comment event payload is here: https://gist.github.com/trentm/17c6a29cf76fdad725e2b119c80a0c36

From a separate discussion some alternative ideas:

  1. Continue to use the issue_comment workflow trigger. Have code that calls the GH API to get the latest commit on the PR before the github.event.comment.created_at time. This feels a little complex, but I think it should work. (Another issue with using issue_comment is that the result of the workflow does not result in GitHub Checks being added to the PR to show whether the TAV tests pass. We would perhaps also need to add usage of the GitHub Checks API to add results.)

  2. Investigate using the pull_request_review_comment event to trigger this workflow. IIUC, this is associated with the current PR commit, so might work much better. It does mean the repo maintainers would need to use the PR review UI for the /test tav trigger comment, but that should be fine.

I played around a little more. The event we can use here is actually pull_request_review (type=submitted). This triggers for any review on a PR. The good thing here is that it is associated with a particular commit on the PR, this means it will test with the commit that was reviewed and there will be GitHub checks progress when using it. Here are a couple screenshots showing that GitHub Checks progress:

Screenshot 2023-04-10 at 2 19 12 PM

Screenshot 2023-04-10 at 2 20 29 PM

Here is a starter "tav-command.yml" used for the above examples:

# Dev notes:
# - An edge case: When I was adding a pull request review for a commit, and
#   a new commit was added to the PR before I submitted it... this did *NOT*
#   result in a `pull_request_review` event. So I guess a "/test tav" will not
#   happen for non-latest commit on the PR. I can live with that.

name: tav-command

on:
  pull_request_review:
    types: [submitted]

# XXX Do we need both of these permissions any more? Because the GitHub checks
#     UI shows progress, we don't need reactions anymore, I don't think.
# permissions:
#   contents: write
#   pull-requests: write

jobs:
  command-validation:
    runs-on: ubuntu-latest
    if: startsWith(github.event.review.body, '/test tav')
    steps:
    - name: XXX Dump GitHub context
      env:
        GITHUB_CONTEXT: ${{ toJson(github) }}
      run: |
        echo "$GITHUB_CONTEXT"

    # XXX This does NOT work on the pull_request_review event data. We will need to support that.
    # - name: Validate github comment
    #   uses: elastic/apm-pipeline-library/.github/actions/validate-github-comment@current

    - uses: actions/checkout@v3
    - run: echo "..."

trentm added a commit that referenced this issue Apr 13, 2023
This replaces a similar script used for when CI was in Jenkins.

Refs: #3227
@trentm
Copy link
Member Author

trentm commented Apr 13, 2023

  • The "dev-utils/jenkins-build-slow-steps.sh" dev tool should be replaced with one that looks at the GH Action runs.

I've started #3267 for this.

trentm added a commit that referenced this issue Apr 18, 2023
This replaces a similar script used for when CI was in Jenkins.

Refs: #3227
@trentm
Copy link
Member Author

trentm commented Apr 18, 2023

  • The "dev-utils/jenkins-build-slow-steps.sh" dev tool should be replaced with one that looks at the GH Action runs. See

This part is done in #3267

@trentm trentm added this to the 8.9 milestone Apr 24, 2023
@v1v v1v self-assigned this May 2, 2023
@v1v
Copy link
Member

v1v commented May 10, 2023

manually run a subset of the TAV tests on a PR

This part is done in #3246

And I tested with #3334 (review) which triggered https://github.com/elastic/apm-agent-nodejs/actions/runs/4933718927

@trentm
Copy link
Member Author

trentm commented May 11, 2023

#3340 for the remaining item

trentm added a commit that referenced this issue May 11, 2023
This checks that .ci/tav.json is correct: both that it covers all the
modules in all the .tav.yml files in the repo, and that it never
results in a TAV test matrix greater than the GH Actions 256 limit.
This is part of `npm run lint` which is run by the lint CI workflow.

This also drops the body-parser TAV tests: we don't instrument it, 
just use it in some tests.

Refs: #3227 (item 3)
@trentm trentm closed this as completed May 11, 2023
trentm added a commit that referenced this issue May 16, 2023
This checks that .ci/tav.json is correct: both that it covers all the
modules in all the .tav.yml files in the repo, and that it never
results in a TAV test matrix greater than the GH Actions 256 limit.
This is part of `npm run lint` which is run by the lint CI workflow.

This also drops the body-parser TAV tests: we don't instrument it, 
just use it in some tests.

Refs: #3227 (item 3)
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this issue Aug 20, 2024
…ic#3267)

This replaces a similar script used for when CI was in Jenkins.

Refs: elastic#3227
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this issue Aug 20, 2024
This checks that .ci/tav.json is correct: both that it covers all the
modules in all the .tav.yml files in the repo, and that it never
results in a TAV test matrix greater than the GH Actions 256 limit.
This is part of `npm run lint` which is run by the lint CI workflow.

This also drops the body-parser TAV tests: we don't instrument it, 
just use it in some tests.

Refs: elastic#3227 (item 3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.9-candidate agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

No branches or pull requests

2 participants