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

Mapping workflow_dispatch inputs into the Expression inputs context #1363

Merged
merged 5 commits into from
Oct 17, 2022

Conversation

KnisterPeter
Copy link
Member

@KnisterPeter KnisterPeter commented Sep 26, 2022

This changes adds the workflow_dispatch event inputs to the inputs context and maintaining the boolean type.

@KnisterPeter KnisterPeter self-assigned this Sep 26, 2022
@KnisterPeter KnisterPeter mentioned this pull request Sep 26, 2022
3 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 3 0 0.02s
✅ REPOSITORY gitleaks yes no 2.48s
✅ REPOSITORY git_diff yes no 0.0s
✅ REPOSITORY secretlint yes no 1.01s

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

@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 27, 2022
@KnisterPeter KnisterPeter changed the title test: check workflow_dispatch inputs Mapping workflow_dispatch inputs into the Expression inputs context Sep 27, 2022
@codecov
Copy link

codecov bot commented Sep 27, 2022

Codecov Report

Merging #1363 (c767290) into master (4f8da0a) will increase coverage by 6.39%.
The diff coverage is 74.70%.

@@            Coverage Diff             @@
##           master    #1363      +/-   ##
==========================================
+ Coverage   57.50%   63.90%   +6.39%     
==========================================
  Files          32       41       +9     
  Lines        4594     6452    +1858     
==========================================
+ Hits         2642     4123    +1481     
- Misses       1729     2034     +305     
- Partials      223      295      +72     
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 45.87% <45.87%> (ø)
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

@KnisterPeter KnisterPeter marked this pull request as ready for review September 27, 2022 10:06
@KnisterPeter KnisterPeter requested a review from a team as a code owner September 27, 2022 10:06
@ChristopherHX
Copy link
Contributor

I'm not shure, but I believe that this change conflics #1345. This raises the question, which should be merged first?

Other than that I want to tell you that github stores all workflow_dispatch input values as strings inside github.event.inputs.* and only respects the boolean type in the inputs.* context.
E.g. github would provide this payload

{
  "inputs": {
    "required": "required input",
    "boolean": "true"
  }
}

@KnisterPeter
Copy link
Member Author

Yes they might conflict. If either one is merged, I'll rework the other. 😊

ChristopherHX
ChristopherHX previously approved these changes Sep 30, 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.

Thank you for adding the missing code to assign the inputs context. You get my +1, even if the test doesn't exactly match githubs behavior.

@KnisterPeter
Copy link
Member Author

You are right regarding the input types. We need to coerce them.

@KnisterPeter
Copy link
Member Author

This should reflect githubs behavior now as well

@mergify mergify bot requested a review from a team October 6, 2022 14:24
ChristopherHX
ChristopherHX previously approved these changes Oct 6, 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.

Looks good to me

cplee
cplee previously approved these changes Oct 6, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 6, 2022

@KnisterPeter this pull request is now in conflict 😩

@mergify mergify bot added the conflict PR has conflicts label Oct 6, 2022
This implements a test to check for `workflow_dispatch` inputs.
This will be a prerequisite for implementing the inputs.
This changes adds the workflow_dispatch event inputs
to the `inputs` context and maintaining the boolean type
@KnisterPeter KnisterPeter dismissed stale reviews from cplee and ChristopherHX via 6a5b1b1 October 17, 2022 07:35
@KnisterPeter KnisterPeter force-pushed the workflow_dispatch_inputs branch from 15ee530 to 6a5b1b1 Compare October 17, 2022 07:35
@mergify mergify bot removed the conflict PR has conflicts label Oct 17, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 17, 2022

@KnisterPeter this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Oct 17, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 17, 2022

@KnisterPeter this pull request has failed checks 🛠

@ChristopherHX
Copy link
Contributor

The PR checks are failing due to actions/hello-world-docker-action@778decc, rerun doesn't help.

@mergify
Copy link
Contributor

mergify bot commented Oct 17, 2022

@KnisterPeter this pull request has failed checks 🛠

@KnisterPeter
Copy link
Member Author

Oh I see. We are depending on code which is not managed by the act team and that did change.
Not a good state I would say.

@KnisterPeter
Copy link
Member Author

I did re-run while the CI indicates that two tests failed (https://github.com/nektos/act/actions/runs/3263285979/jobs/5362282276#step:6:33010) and both are fine locally. But anyway if the error lies in the external code I guess I will wait with this PR until its fixed.

@ChristopherHX
Copy link
Contributor

... both are fine locally

You would need to flush your docker image cache (some stale images with act in it's name) to observe this locally, act currently never rebuilds docker actions. The CI always needs to build all docker actions.

@mergify mergify bot removed the needs-work Extra attention is needed label Oct 17, 2022
@mergify mergify bot requested a review from a team October 17, 2022 16:15
@mergify mergify bot merged commit 1e0ef8c into master Oct 17, 2022
@mergify mergify bot deleted the workflow_dispatch_inputs branch October 17, 2022 16:25
@itsjoekent
Copy link

Any chance this can get released soon? Looks like the last release was 19 days ago

@KnisterPeter
Copy link
Member Author

There is one usual release per month

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.

4 participants