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

fix: keep path to event json file in composite actions #1428

Merged
merged 4 commits into from
Nov 16, 2022

Conversation

KnisterPeter
Copy link
Member

@KnisterPeter KnisterPeter commented Nov 7, 2022

The event.json paths need to be copied over, since it the
GithubContext is recreated from the composite RC. And that
does read some value from the event file if available.

Fixes #1401

The event.json paths need to be copied over, since it the
GithubContext is recreated from the composite RC. And that
does read some value for the event file if available.
@KnisterPeter KnisterPeter added the kind/bug Something isn't working label Nov 7, 2022
@KnisterPeter KnisterPeter self-assigned this Nov 7, 2022
@KnisterPeter KnisterPeter requested a review from a team as a code owner November 7, 2022 15:16
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 1 0 0.01s
✅ REPOSITORY gitleaks yes no 2.82s
✅ REPOSITORY git_diff yes no 0.0s
✅ REPOSITORY secretlint yes no 1.35s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #1428 (7f36231) into master (4f8da0a) will increase coverage by 6.23%.
The diff coverage is 74.29%.

@@            Coverage Diff             @@
##           master    #1428      +/-   ##
==========================================
+ Coverage   57.50%   63.74%   +6.23%     
==========================================
  Files          32       41       +9     
  Lines        4594     6476    +1882     
==========================================
+ Hits         2642     4128    +1486     
- Misses       1729     2049     +320     
- Partials      223      299      +76     
Impacted Files Coverage Δ
pkg/common/file.go 0.00% <0.00%> (ø)
pkg/model/action.go 0.00% <0.00%> (ø)
pkg/model/step_result.go 0.00% <ø> (ø)
pkg/container/docker_run.go 12.82% <11.58%> (+7.27%) ⬆️
pkg/model/workflow.go 45.65% <23.80%> (-5.26%) ⬇️
pkg/container/docker_pull.go 33.33% <33.33%> (ø)
pkg/container/file_collector.go 43.11% <43.11%> (ø)
pkg/common/git/git.go 50.00% <49.01%> (ø)
pkg/container/docker_auth.go 47.61% <50.00%> (+2.61%) ⬆️
pkg/model/planner.go 48.82% <50.00%> (-1.60%) ⬇️
... and 33 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ChristopherHX
Copy link
Contributor

ChristopherHX commented Nov 7, 2022

Add this test? You can append the steps to the test for github.head_ref github.base_ref

on: push
jobs:
  _:
    runs-on: self-hosted
    container: ubuntu:latest
    steps:
    - run: |
        runs:
          using: composite
          steps:
          - run: |
              echo WORKFLOW_GITHUB_CONTEXT="$WORKFLOW_GITHUB_CONTEXT"
              echo COMPOSITE_GITHUB_CONTEXT="$COMPOSITE_GITHUB_CONTEXT"
              [[ "$WORKFLOW_GITHUB_CONTEXT" = "$COMPOSITE_GITHUB_CONTEXT" ]]
            env:
              WORKFLOW_GITHUB_CONTEXT: ${{ tojson(tojson(github.event)) }}
              COMPOSITE_GITHUB_CONTEXT: ${{ '${{tojson(github.event)}}' }}
            shell: bash
      shell: cp {0} action.yml
    - uses: ./

I verified that this change fixes the issue and this test will only pass with a user provided event.json file with this patch.

  • WORKFLOW_GITHUB_CONTEXT is hardcoded into the action and has the value of the workflow level github.event.
  • COMPOSITE_GITHUB_CONTEXT is evaluated inside the action

Both should be equal.

ChristopherHX
ChristopherHX previously approved these changes Nov 7, 2022
Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

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

I prefer that this includes a test, like the one in my comment. However this should be fixed asap.

@KnisterPeter
Copy link
Member Author

You are right that we need a test for this. It's quite important.

@mergify
Copy link
Contributor

mergify bot commented Nov 7, 2022

@KnisterPeter this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Nov 7, 2022
@KnisterPeter
Copy link
Member Author

The test from above doesn't seem to work as expected.

@ChristopherHX
Copy link
Contributor

Copy pasting a test is not trivial, I will fix your yaml errors

@KnisterPeter
Copy link
Member Author

Thanks 😂

@mergify mergify bot removed the needs-work Extra attention is needed label Nov 7, 2022
@mergify mergify bot merged commit 2614b3e into master Nov 16, 2022
@mergify mergify bot deleted the keep-event-json-path branch November 16, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event details not loaded from JSON file provided via --eventpath param
3 participants