-
-
Notifications
You must be signed in to change notification settings - Fork 629
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
feat: Make CHECKSUM and TIMESTAMP vars available in cmds commands #1872
feat: Make CHECKSUM and TIMESTAMP vars available in cmds commands #1872
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello
Thanks for your PR, I've left some comments!
@@ -228,7 +181,7 @@ func (e *Executor) compiledTask(call *ast.Call, evaluateShVars bool) (*ast.Task, | |||
} | |||
} | |||
|
|||
if len(origTask.Status) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of those two nested if statements.
TIMESTAMP
and CHECKSUM
should only be available if there are sources defined. I'd replace len(origTask.Status) > 0
with len(origTask.Source) > 0
and move it above cmds
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, good idea. I changed it like you said but now it also has the effect that .TIMESTAMP
and .CHECKSUM
are available in preconditions
and deps
. Do you want to keep it like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed this is wanted 🙂
104542a
to
370d528
Compare
Lint job is failing. a |
370d528
to
856aa66
Compare
856aa66
to
7d9a093
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
Hello, this is my attempt to fix #1735.