-
-
Notifications
You must be signed in to change notification settings - Fork 634
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: run once in shared dependencies #1655
Conversation
In my option, the number 2 is really dangerous. I would prefer a task running twice even it's not needed, than a task not running at all when it should it's a tough topic. I would go for the 1 or 3 |
Agreed
I've given this more thought and I'm inclined to agree for now. I can see the potential upsides and downsides of option 3, but for now I think leaving this PR as-is and fixing the first issue is a safe bet. I'm still open to discussion about the best way to solve the second problem in another issue. |
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.
What do you think about adding a test for this usecase ?
I manage to test it (to ensure it work as expected)
I've added it to the run
testdata
Something like this :
library.yml
version: 3
tasks:
build:
run: once
cmds:
- echo 'Building library...'
service.yml
version: 3
includes:
library:
taskfile: library.yml
tasks:
build:
deps: [library:build]
cmds:
- echo 'Building service {{.NAME}}...'
The root taskfile:
includes:
service-a:
taskfile: service.yml
vars:
NAME: "service-a"
service-b:
taskfile: service.yml
vars:
NAME: "service-b"
tasks:
build:
silent: false
cmds:
- task: service-a:build
- task: service-b:build
Then we need to verify that Building library...
is printed only once.
I can add or you can add, or you can discard my comment. It's your call here :)
Thanks for the work :)
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.
Nice! 👏
I agree with @vmaerten that adding a test before merging would be nice.
Fixes #852
This PR changes the hash we use when a task is marked as
run:once
. Previously we used the full name of the task. This is fine most of the time, but when a task in an included Taskfile is included multiple times, the name will contain a namespace that is different for each task. Using the example given in #852, the hashes produced are:As a result
library:build
is run twice, even though it is marked asrun: once
.In this PR, we are creating a hash that is a combination of the file location and the name of the task without a namespace. This means that the hash is unique for each task, but will be the same when a task is included in different namespaces.
Note
As discussed below, we're no longer focussing on the second problem in this PR. We're open to further discussion in another issue for anyone coming across this in the future.
#852 mentions a secondary problem:
You can see this illustrated below:
We need to determine the correct behaviour:
run: once
is specified - This still has the same problem as above. However, you could argue that the user has made a mistake by addingrun: once
when the result of a task could be different depending on the context.If we choose 2 or 3, then what should the task hash be?
<location>:service-a:build
(probably not)<location>:service-b:build
(also probably not)I would appreciate any thoughts/feedback on these options - or other options entirely.