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

WorkflowDispatchConfig supports multiple yaml node kinds #2123

Merged
merged 4 commits into from
Jan 20, 2024
Merged

WorkflowDispatchConfig supports multiple yaml node kinds #2123

merged 4 commits into from
Jan 20, 2024

Conversation

pangliang
Copy link
Contributor

@pangliang pangliang commented Dec 12, 2023

I was working on PR go-gitea/gitea#28163 . and found that the workflow.WorkflowDispatchConfig() method cannot be parsed correctly when the yaml node is not a mapping type

on: workflow_dispatch

on: [push, workflow_dispatch]

on:
    - push
    - workflow_dispatch

on:
    push:
    pull_request:
    workflow_dispatch:
        inputs:
            logLevel:
                description: 'Log level'
                required: true
                default: 'warning'
                type: choice
                options:
                - info
                - warning
                - debug

@wolfogre
Copy link
Member

Sorry, are you sure your cases are supported by GitHub? Have you tested them?

image

@pangliang
Copy link
Contributor Author

Sorry, are you sure your cases are supported by GitHub? Have you tested them?

image

Yes, github has a special check and does not support nested SequenceNode; but the parsing method supports nesting to be more robust; what do you think?

@wolfogre
Copy link
Member

TBH, I don't think it's more robust, it will cause inconsistent behavior. And I don't see any reason why it should support - [pull_request, workflow_dispatch].

@pangliang
Copy link
Contributor Author

TBH, I don't think it's more robust, it will cause inconsistent behavior. And I don't see any reason why it should support - [pull_request, workflow_dispatch].

I checked the yaml spec, chapter Example 2.5 Sequence of Sequences, The yaml language does have this structure.

So I think this parse method is flow yaml spec, which is better than following the special restrictions of github; especially when this act will be widely used by other gitxxx.

It's just my suggestion, but if you think it's better to follow github, I'll just delete the nested processing part.

@wolfogre
Copy link
Member

Please let me clarify, I'm not talking about the yaml specification. Of course, - [pull_request, workflow_dispatch] is supported by yaml, or else you wouldn't be able to parse it with gopkg.in/yaml.v3. I think we are discussing the workflow syntax.

@pangliang
Copy link
Contributor Author

Okay, here I flow github and add support for the following types:

on: workflow_dispatch

on: [push, workflow_dispatch]

on:
    - push
    - workflow_dispatch

on:
    push:
    pull_request:
    workflow_dispatch:
        inputs:
            logLevel:
                description: 'Log level'
                required: true
                default: 'warning'
                type: choice
                options:
                - info
                - warning
                - debug

@pangliang
Copy link
Contributor Author

There is another change. When workflow_dispatch is not defined, return nil

on:
    push:

Such cases pre returned non-nil

pkg/model/workflow.go Outdated Show resolved Hide resolved
pkg/model/workflow.go Outdated Show resolved Hide resolved
wolfogre
wolfogre previously approved these changes Dec 13, 2023
Copy link
Contributor

mergify bot commented Dec 13, 2023

@pangliang this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Dec 13, 2023
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Attention: 1026 lines in your changes are missing coverage. Please review.

Comparison is base (4989f44) 61.22% compared to head (f97f497) 60.91%.
Report is 302 commits behind head on master.

Files Patch % Lines
pkg/artifactcache/handler.go 65.32% 103 Missing and 43 partials ⚠️
pkg/runner/run_context.go 73.37% 75 Missing and 19 partials ⚠️
pkg/runner/expression.go 55.17% 66 Missing and 12 partials ⚠️
pkg/runner/action_cache.go 50.74% 49 Missing and 17 partials ⚠️
pkg/container/docker_run.go 1.51% 64 Missing and 1 partial ⚠️
pkg/container/docker_network.go 0.00% 56 Missing ⚠️
pkg/model/planner.go 28.57% 53 Missing and 2 partials ⚠️
pkg/model/workflow.go 52.72% 42 Missing and 10 partials ⚠️
pkg/runner/reusable_workflow.go 52.47% 42 Missing and 6 partials ⚠️
pkg/common/outbound_ip.go 0.00% 44 Missing ⚠️
... and 27 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2123      +/-   ##
==========================================
- Coverage   61.22%   60.91%   -0.31%     
==========================================
  Files          46       53       +7     
  Lines        7141     8988    +1847     
==========================================
+ Hits         4372     5475    +1103     
- Misses       2462     3072     +610     
- Partials      307      441     +134     

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

@mergify mergify bot removed the needs-work Extra attention is needed label Dec 14, 2023
@pangliang pangliang requested a review from cplee January 20, 2024 11:44
@pangliang
Copy link
Contributor Author

pangliang commented Jan 20, 2024

Sorry, I accidentally clicked the 'Re-request review' icon in the 'Reviewers' list of a pull-request page

@mergify mergify bot merged commit 8072a00 into nektos:master Jan 20, 2024
10 checks passed
jmikedupont2 pushed a commit to meta-introspector/act that referenced this pull request Mar 10, 2024
* WorkflowDispatchConfig supports ScalarNode and SequenceNode yaml node kinds

* Avoid using log.Fatal

* package slices is not in golang 1.20

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.

3 participants