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: prevent unintended input replacement in reusable workflows with workflow_dispatch when using workflow_call #2502

Merged
merged 15 commits into from
Dec 29, 2024

Conversation

mahmud2011
Copy link
Contributor

Closes: #2464

@mahmud2011 mahmud2011 requested a review from a team as a code owner October 24, 2024 14:22
@mahmud2011 mahmud2011 changed the title fix: prevent replacing inputs in reusable workflow in workflow_call with workflow_dispatch fix: prevent unintended input replacement in reusable workflows with workflow_dispatch when using workflow_call Oct 24, 2024
@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 24, 2024
@ChristopherHX
Copy link
Contributor

ChristopherHX commented Oct 24, 2024

FYI: This reverts a feature that has been accepted by the owner in #1845 that has 9 upvotes in the original issue. fixed

@pull-request-size pull-request-size bot added size/S and removed size/M labels Oct 25, 2024
@mahmud2011
Copy link
Contributor Author

mahmud2011 commented Oct 25, 2024

@ChristopherHX I reverted it.

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.

Yes I confirm that you no longer remove the feature I mentioned.

Could you please add a pkg/runner/testdata/<testname>/workflow_dispatch.yml and a reusable workflow in pkg/runner/testdata/.github/workflows/<testworkflow>.yml that assert that your change works now and in a year?

Then a new line like

{workdir, "workflow_dispatch", "workflow_dispatch", "", platforms, secrets},

where the first workflow_dispatch should be replaced by <testname>

in the workflow_dispatch.yml do a

uses: ./.github/workflows/<testworkflow>.yml

see uses https://github.com/nektos/act/tree/master/pkg/runner/testdata/uses-workflow for an example.

didn't test yet as you always need two approvals, I take me some time for actually testing this

Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.75%. Comparing base (5a80a04) to head (4fabbc7).
Report is 140 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2502      +/-   ##
==========================================
+ Coverage   61.56%   68.75%   +7.19%     
==========================================
  Files          53       71      +18     
  Lines        9002    10918    +1916     
==========================================
+ Hits         5542     7507    +1965     
+ Misses       3020     2848     -172     
- Partials      440      563     +123     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mergify mergify bot requested a review from a team October 25, 2024 18:42
@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 27, 2024
@mergify mergify bot added the needs-work Extra attention is needed label Oct 27, 2024
@nektos nektos deleted a comment from mergify bot Oct 27, 2024
@mergify mergify bot removed the needs-work Extra attention is needed label Oct 27, 2024
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.

Added you sample workflow as test with an assert step all good now

@mahmud2011
Copy link
Contributor Author

Thanks. I was about to write the tests.

@ChristopherHX
Copy link
Contributor

I was about to write the tests.

More tests are welcome, I reapprove if needed as the most active member.

However we need to wait for one other member to make a merge happen.

@ChristopherSchmidt89
Copy link

Hi, it would be highly appreciated if this fix gets another review to resolve the mentioned issue!
Thanks

@blackhatcrazy
Copy link

likewise, this would help our dev setup significantly! Please bring this in soon.

Copy link
Contributor

mergify bot commented Dec 28, 2024

@mahmud2011 this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Dec 28, 2024
@mergify mergify bot removed the needs-work Extra attention is needed label Dec 29, 2024
@mergify mergify bot merged commit deea8ec into nektos:master Dec 29, 2024
11 checks passed
@mahmud2011 mahmud2011 deleted the bugfix/2464 branch December 29, 2024 15:00
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.

Reusable workflow uses workflow_dispatch default instead of passed input value
5 participants