Feature/file.fetch #543
Feature/file.fetch #543
Conversation
| status = resource.NewStatus() | ||
| ) | ||
|
|
||
| if !f.hasApplied { |
There was a problem hiding this comment.
Could you do if f.hasApplied { return status, nil } here instead to reduce nesting of the rest of the code?
| stat, err := f.DiffFile(status, hsh) | ||
| if err != nil { | ||
| return stat, err | ||
| } else if resource.AnyChanges(stat.Differences) { |
There was a problem hiding this comment.
Would it be an option here to do else if !resource.AnyChanges(state.Differences) { return status, nil }?
| status.AddMessage("fetched successfully") | ||
| f.hasApplied = true | ||
| } | ||
|
|
There was a problem hiding this comment.
I wonder if it might be worth it here to actually set f.Hash even if the user hadn't originally specified one- the idea being that someone might want to do a lookup on the hash of a fetched file even if they didn't actually specify an explicit one. What do you think?
There was a problem hiding this comment.
We can't actually do this without knowing the hash type (md5, sha1, etc.).
| } | ||
|
|
||
| if p.Hash != nil { | ||
| if strings.TrimSpace(*p.Hash) == "" { |
There was a problem hiding this comment.
Is it worth actually checking the length of the string here to make sure it's the right length for a hash of the specified type?
arichardet
force-pushed
the
feature/file.fetch
branch
from
6f8658e
to
ee20e07
Compare
|
LGTM after vendor fixing and rebasing on #533 |
BrianHicks
force-pushed
the
feature/file.fetch
branch
from
ee20e07
to
247bc99
Compare
arichardet
force-pushed
the
feature/file.fetch
branch
from
247bc99
to
addf699
Compare
Add file.fetch module: