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

Fixes #1387 #1388

Merged
merged 1 commit into from
Oct 18, 2022
Merged

Fixes #1387 #1388

merged 1 commit into from
Oct 18, 2022

Conversation

zukwung
Copy link
Contributor

@zukwung zukwung commented Oct 13, 2022

This fixes #1387, or an attempt at it... please let me know if there are any changes required. Thanks!

@zukwung zukwung requested a review from a team as a code owner October 13, 2022 15:03
ChristopherHX
ChristopherHX previously approved these changes Oct 13, 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.

If I think more about the GITHUB_ENV file parser, act is still to restrictive and should allow all chars other than \n and \r as a delimitter after a << token.

@zukwung
Copy link
Contributor Author

zukwung commented Oct 13, 2022

If I think more about the GITHUB_ENV file parser, act is still to restrictive and should allow all chars other than \n and \r as a delimitter after a << token.

I can update it to be .+ if you want @ChristopherHX, up to you

@codecov
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

Merging #1388 (e612e63) into master (4f8da0a) will increase coverage by 6.34%.
The diff coverage is 74.60%.

❗ Current head e612e63 differs from pull request most recent head 5cdea32. Consider uploading reports for the commit 5cdea32 to get more accurate results

@@            Coverage Diff             @@
##           master    #1388      +/-   ##
==========================================
+ Coverage   57.50%   63.85%   +6.34%     
==========================================
  Files          32       41       +9     
  Lines        4594     6452    +1858     
==========================================
+ Hits         2642     4120    +1478     
- Misses       1729     2036     +307     
- Partials      223      296      +73     
Impacted Files Coverage Δ
pkg/model/action.go 0.00% <0.00%> (ø)
pkg/model/step_result.go 0.00% <ø> (ø)
pkg/container/docker_run.go 12.82% <11.63%> (+7.27%) ⬆️
pkg/model/workflow.go 47.14% <22.22%> (-3.77%) ⬇️
pkg/model/planner.go 48.82% <25.00%> (-1.60%) ⬇️
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% <47.91%> (ø)
pkg/container/docker_auth.go 47.61% <50.00%> (+2.61%) ⬆️
pkg/exprparser/interpreter.go 73.37% <53.48%> (-0.02%) ⬇️
... and 31 more

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

@ChristopherHX
Copy link
Contributor

Thank you it's fine as is from my side, it's good a have one more test for the GITHUB_ENV file.

Act has more defects in that code, like this one:

on: push
jobs:
  _:
    runs-on: ubuntu-latest
    steps:
    - run: |
        echo "test<<World" > $GITHUB_ENV
        echo "x=Thats really Weird" >> $GITHUB_ENV
        echo "World" >> $GITHUB_ENV
    - if: env.test != 'x=Thats really Weird'
      run: exit 1
    - if: env.x == 'Thats really Weird' # This assert is triggered by the broken impl of act
      run: exit 1

just the existing method feels like to need a rewrite ( be aligned to actions/runner's code ) and be factored out into a reusable method.

@zukwung
Copy link
Contributor Author

zukwung commented Oct 13, 2022

just the existing method feels like to need a rewrite ( be aligned to actions/runner's code ) and be factored out into a reusable method.

I'll leave that to the experts 😎 . Is there anything else I need to do before we can merge this fix?

@ChristopherHX
Copy link
Contributor

Get two more approvals from other maintainers.

KnisterPeter
KnisterPeter previously approved these changes Oct 17, 2022
@mergify mergify bot merged commit bc17371 into nektos:master Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiline Environment variables isn't propagated by act between steps
4 participants