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: fail if no stages were found #1970

Merged
merged 4 commits into from
Aug 21, 2023

Conversation

ctrlaltf24
Copy link
Contributor

Adds a warning message if act is cannot find any stages to run with the filters provided.

Reproduction:

  • run act -j gibberish

Desired behavior: some indication I did something silly
Actual behavior: no output, just exit with success.

As a human who often makes spelling mistakes,
it would be nice if act warned me what I was doing that was silly rather than exiting apparently doing
nothing with no obvious indication
I did something wrong.

Alternative solutions:

  • Do nothing, it's the user's fault: True it is, would be nice to give them a hint where to resolve their issue
  • Exit with error rather than warn: Reasonable behavior as well, not knowledgeable enough with the code base to know for sure this is enough justification to exit

Adds a warning message if act is cannot find any stages to run
with the filters provided.

Reproduction:
- run `act -j gibberish`

Desired behavior: some indication I did something silly
Actual behavior: no output, just exit with success.

As a human who often makes spelling mistakes,
it would be nice if act warned me what I was doing that was silly
rather than exiting apparently doing
nothing with no obvious indication
I did something wrong.
@ctrlaltf24 ctrlaltf24 requested a review from a team as a code owner August 15, 2023 01:33
Copy link
Contributor

@ZauberNerd ZauberNerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make more sense to return an error from here:

act/pkg/model/planner.go

Lines 331 to 358 in 7ba9f30

var err error
// next, build an execution graph
stages := make([]*Stage, 0)
for len(jobDependencies) > 0 {
stage := new(Stage)
for jID, jDeps := range jobDependencies {
// make sure all deps are in the graph already
if listInStages(jDeps, stages...) {
stage.Runs = append(stage.Runs, &Run{
Workflow: w,
JobID: jID,
})
delete(jobDependencies, jID)
}
}
if len(stage.Runs) == 0 {
return nil, fmt.Errorf("unable to build dependency graph for %s (%s)", w.Name, w.File)
}
stages = append(stages, stage)
}
if len(stages) == 0 && err != nil {
return nil, err
}
return stages, nil
}

As you can see the err variable is unused, so I'd probably remove it and use the if len(stages) == 0 to return nil, fmt.Errorf("...")

@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2023

@ctrlaltf24 this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Aug 17, 2023
Errors if no stages were found with the given filters.
Prints out a helpful error message, pointing users
in the right place for how to specify which stage to run.

Reproduction:
- run `act -j gibberish`

Desired behavior: some indication I did something silly
Actual behavior: no output, just exit with success.

As a human who often makes spelling mistakes,
it would be nice if act warned me what I was doing that was silly
rather than exiting apparently doing
nothing with no obvious indication
I did something wrong.
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #1970 (e6a2177) into master (4989f44) will increase coverage by 0.90%.
Report is 227 commits behind head on master.
The diff coverage is 61.98%.

@@            Coverage Diff             @@
##           master    #1970      +/-   ##
==========================================
+ Coverage   61.22%   62.12%   +0.90%     
==========================================
  Files          46       52       +6     
  Lines        7141     8484    +1343     
==========================================
+ Hits         4372     5271     +899     
- Misses       2462     2794     +332     
- Partials      307      419     +112     
Files Changed Coverage Δ
pkg/common/outbound_ip.go 0.00% <0.00%> (ø)
pkg/container/docker_cli.go 82.23% <ø> (ø)
pkg/container/docker_logger.go 52.08% <ø> (ø)
pkg/container/docker_volume.go 0.00% <0.00%> (ø)
pkg/container/file_collector.go 37.30% <0.00%> (ø)
pkg/container/host_environment.go 0.00% <0.00%> (ø)
...ontainer/linux_container_environment_extensions.go 23.07% <0.00%> (-1.25%) ⬇️
pkg/container/util.go 0.00% <0.00%> (ø)
pkg/exprparser/functions.go 66.32% <0.00%> (-1.04%) ⬇️
pkg/container/docker_run.go 13.45% <6.66%> (-0.13%) ⬇️
... and 29 more

... and 2 files with indirect coverage changes

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

@mergify mergify bot removed the needs-work Extra attention is needed label Aug 21, 2023
@mergify mergify bot merged commit 7286b43 into nektos:master Aug 21, 2023
10 checks passed
@ctrlaltf24 ctrlaltf24 deleted the user-facing-debug-error-when-silly branch August 23, 2023 23:57
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.

None yet

3 participants