throw when invalid uses key is provided
#1804
Conversation
|
@JoshMcCullough this pull request has failed checks 🛠 |
|
@JoshMcCullough this pull request has failed checks 🛠 |
Codecov Report
@@ Coverage Diff @@
## master #1804 +/- ##
==========================================
+ Coverage 61.22% 62.58% +1.35%
==========================================
Files 46 51 +5
Lines 7141 8282 +1141
==========================================
+ Hits 4372 5183 +811
- Misses 2462 2701 +239
- Partials 307 398 +91
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
JoshMcCullough
force-pushed
the
jsm/fix-workflow-jobtype
branch
from
17970ab
to
237ea80
Compare
JoshMcCullough
force-pushed
the
jsm/fix-workflow-jobtype
branch
from
237ea80
to
e874540
Compare
JoshMcCullough
changed the title
uses key is provided
|
@JoshMcCullough this pull request has failed checks 🛠 |
There was a problem hiding this comment.
I'm dropping my changes requested review, somehow I'm not allow to dismis my review.
As long the linter complains about your tests, merging is blocked
I guess you need either add
// nolint:dupl
func TestReadWorkflow_JobTypes(t *testing.T) {
...
// nolint:dupl
func TestReadWorkflow_JobTypes_InvalidPath(t *testing.T) {
Or merge the test functions to make the linter (I didn't added it) happy.
You can download golangci-lint and run it locally in your checkout
|
@JoshMcCullough this pull request has failed checks 🛠 |
|
@JoshMcCullough this pull request has failed checks 🛠 |
|
@JoshMcCullough this pull request has failed checks 🛠 |
I actually did try to run that, it gives me a massive amount of superfluous output so I didn't trust it. :) I promise I read Those tests are not actually duplicative so I don't know why the linter thinks they are...seems a bit picky. |
|
Not sure what lint is upset about here:
I also don't see why the Linux build is so mad. :) |
Warnings like this seems to raised as errors
Flaky tests, maybe we should disable TestRunEvent/network. Sometime the container cannot resolve the dns name of the hosted runner |
|
Finally. |
pkg/model/workflow.go
Outdated
| isYaml, _ := regexp.MatchString(`\.(ya?ml)(?:$|@)`, j.Uses) | ||
|
|
||
| if isYaml { | ||
| isLocalPath := strings.HasPrefix(j.Uses, "./.github/workflows/") |
There was a problem hiding this comment.
The gitea people probably want ./.gitea/workflows/ to work, but they can just patch it in their fork.
I would just allow any path for reusable workflows regardless of the folder, but this error helps most people using this tool
There was a problem hiding this comment.
I was also thinking it shouldn't matter where the workflow YML is. But when I was using "any path" e.g. under a totally different/custom root dir in the repo, this was causing issues. It's possible that it was actually find and the issue was something else.
I'll push a change for this.
|
Looks fine for me. |
| if !runJob { | ||
| l.WithField("jobResult", "skipped").Debugf("Skipping job '%s' due to '%s'", job.Name, job.If.Value) | ||
| return false, nil | ||
| } | ||
|
|
||
| if job.Type() != model.JobTypeDefault { |
There was a problem hiding this comment.
Oops the else if jobType != model.JobTypeDefault { block needs to be moved back, because it should allow skipping reusable workflows.
While jobtypeinvalid should return an error as early as possible.
This means we don't have a test for skipping reusable workflows
Do you accept the regression I mentioned above ? If so I approve, since I don't use the reusable workflow feature of act. |
|
@JoshMcCullough this pull request has failed checks 🛠 |
Originally I thought the path checks were too specific, and the a reusable workflow didn't need to be stored under
.github/workflows/. I believe I was confusing actions with workflows while reading the docs. So in the end, this PR ends up being simply to throw/exit in the case that an invalidusesis provided.