refactor: re-organize node loading code to make it easier to follow #1771
Conversation
There was a problem hiding this comment.
Hi @pbitty!
I like the idea, but there a couple of thing that needs to be addressed before merging:
- Tests are failing unless
TASK_X_REMOTE_TASKFILES=1is set - Can you re-write the commit messages to comply with Conventional Commits?
Thanks, will do!
Ha, I missed this because I have set it permanently in my shell environment. I figure the best place to add it is in the Go tests themselves. Let me know if you have another idea. |
pbitty
force-pushed
the
cache-reorganize
branch
from
f301cfc to
353477b
Compare
task_test.go
Outdated
| // overrideExperimentValue allows one to change the value of an experiment value for a set of tests, | ||
| // with the value being restored to its previous state when tests complete. | ||
| // | ||
| // Typically experiments are controlled via TASK_X_ env vars, but we cannot use those in tests | ||
| // because the experiment settings are parsed during experiments.init(), before any tests run. | ||
| func overrideExperimentValue(t *testing.T, e *experiments.Experiment, enabled bool, val string) { | ||
| prev := *e | ||
| *e = experiments.Experiment{ | ||
| Name: prev.Name, | ||
| Enabled: enabled, | ||
| Value: val, | ||
| } | ||
| t.Cleanup(func() { *e = prev }) | ||
| } |
There was a problem hiding this comment.
@andreynering Are you okay with this work-around? I figure anything deeper would require changing the experiments package and it doesn't seem worth it to accommodate tests.
There was a problem hiding this comment.
I think this is good.
Imho, we do not need the enabled param, it'll be always true, as all experiments are disabled by default
There was a problem hiding this comment.
Good point. I will adjust it.
|
@andreynering , I did a force-push in order to alter the commit messages. Do they look ok? I could also squash all the commits down to a single one, and call it or something along those lines. |
|
Hey @pbitty ! I would like to merge #1713 first as it solves one issue (Timeout message is not correct) |
pbitty
changed the title
pbitty
changed the title
|
Just noting that I haven't forgotten about this, just trying to find some time this week to do it. |
Readability is subjective, but while working in this part of the code I found it difficult to follow the different code paths when loading a node's content. In this change I am attempting to make the branches of the decision tree return early as much as possible. This way we discard each possibility as we work our way down, in this order: 1. Non-remote nodes 2. In offline mode, use cached remote nodes 3. On network timeout, use cached remote nodes 4a. Use fetched remote node 4b. On checksum change, handle writing to cache Tests are added to ensure this functionality does not break.
pbitty
force-pushed
the
cache-reorganize
branch
from
37232b7 to
ab18f5e
Compare
This new package pre-seeds the global rand source on every process start so we don't need to maintain our own rand.Source variable.
|
@andreynering , @vmaerten, the rebase is done! Ready for merge. The I added one more commit which uses |
|
Thanks! |
While working in this part of the code I found it difficult to follow the different code paths when loading
a node's content. I realize readability is subjective, but I am suggesting this change in case you also find it improves readability and maybe maintainability.
In this change I am attempting to make the branches of the decision tree return early as much as possible. This way we discard each possibility as we work our way down, in this order:
b. On checksum change, handle writing to cache
Tests are added to ensure this functionality does not break.
I broke up the changes into logical commits that are fairly isolated. It might be easier to review them individually.
What do you think?
(I got into this code path while exploring how to incorporate go-getter into the remote taskfile feature (as proposed in this comment). Thanks for maintaining this tool!)