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

Printing out why a stage is running #197

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

timmattison
Copy link

More low hanging fruit?

For #100

@kevin-hanselman
Copy link
Owner

Thanks for the PRs! I'm aiming to have a look at these this weekend.

if hasCommand && (len(stg.Inputs) == 0) {
// Run if we have a command and no inputs.
doRun = true
logger.Info.Printf("running stage %s because there is a command and no inputs\n", stagePath)
Copy link
Owner

Choose a reason for hiding this comment

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

Could we have a reason string that sets a brief description, and then a single log line that prints the reason? I like enforcing the pattern of displaying running stage STAGE_NAME (BRIEF DESCRIPTION).

This also highlights some missed opportunities to short circuit the "should run?" check. I think most of this function should be moved into a checkRun function (or similar name--haven't thought too hard about it). That way it can simply return to skip unnecessary checks. checkRun would return doRun and reason.

How do you feel about tackling this?

// Run if our checksum is stale.
doRun = true
logger.Info.Printf("running stage %s because the checksum is not up to date\n", stagePath)
}
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like you missed a couple places where doRun is potentially set. ⬇️

Also I'd like to see some tests for this. I'd be fine with either traditional tests (run_test.go 😄) or my "integration" tests, which are just shell scripts with some machinery built around them; peek around in tests/integration/ to see some examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants