-
-
Notifications
You must be signed in to change notification settings - Fork 624
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
Disable version check for external libraries #1938
base: main
Are you sure you want to change the base?
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.
Thanks for this @leaanthony.
Just a thought before this is merged. I don't think we should require users of our library to set a DisableVersionCheck
flag to make Task work as package. I feel like this should be the default behaviour and our CLI entrypoint should enable it instead (basically, the reverse of this PR).
I actually agree with you. This got me over the line but yeah adding the in the CLI and not |
@pd93 - Updated the PR to run the version initialisation on cli startup. Tidied the version output if there is no sum. |
internal/version/version.go
Outdated
if sum == "" { | ||
sum = info.Main.Sum | ||
} | ||
return | ||
} |
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.
We need to keep these empty checks as these variables are pre-populated by ldflags
in some package managers. The default value also needs to be empty for the same reason.
There have been quite a few accidental changes around this function. We should probably add some comments here to explain this too.
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.
Coming at it fresh it does seem a bit strange 😅 The reason for the change was a failing test that assumed it would be "unknown" but would return blank. What's the best approach to fix that? Set it in Init if it's not set in the buildinfo?
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.
This is because our tests treat Task as a package, not a CLI. This is a bit awkward to fix, but I think we could add a setting in the executor called EnableVersionCheck
(Similar to the DisableVersionCheck
you had previously). This setting would then need to be added to the CLI entrypoint and any relevant tests that need the version check functionality.
504a0da
to
89e7b35
Compare
This PR adds an internal flag to the executor to disable version checks. This is to ensure that any application that uses Task as a library doesn't error when Task checks the buildinfo.
This PR also renames
close
tocloser
intask.go
as it clashes with the keyword `close.