[Binary provisioning] Exclude fallback path #4730
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
codebien
force-pushed
the
binp/general-improvements
branch
from
506e47e to
d3a17d6
Compare
pablochacin
reviewed
Apr 24, 2025
internal/cmd/launcher.go
Outdated
| reportIssues := "If you encounter any unexpected user experience or something doesn't go as planned," + | ||
| " please report it by creating an issue." | ||
|
|
||
| l.gs.Logger.Info("Experimental feature for Binary provisioning is enabled." + |
There was a problem hiding this comment.
In #4731 we removed this information message.
codebien
force-pushed
the
binp/general-improvements
branch
from
d3a17d6 to
3c5bb93
Compare
codebien
force-pushed
the
binp/general-improvements
branch
from
3c5bb93 to
504e761
Compare
pablochacin
reviewed
Apr 24, 2025
internal/cmd/launcher.go
Outdated
| Error("Failed to provision a k6 binary with required dependencies." + | ||
| " Please, make sure to report this issue by opening a bug report.") | ||
| Error("Failed to provision a k6 binary with required dependencies. " + | ||
| reportIssues) |
There was a problem hiding this comment.
We shouldn't reuse report Issues here as it will produce this weird message. We are reporting an error and still saying If you encounter any unexpected user experience or something didn't go as planned
ERRO[0000] Failed to provision a k6 binary with required dependencies. If you encounter any unexpected user experience or something didn't go as planned, please report it by creating an issue. error="invalid build parameters: cannot satisfy dependency : k6/x/faker >0.5.0"
Suggested change
| reportIssues) | |
| " Please, make sure to report this issue by opening a bug report.") |
pablochacin
reviewed
Apr 24, 2025
internal/cmd/launcher.go
Outdated
| Debug("Binary provisioning feature is enabled.") | ||
| Debug("Binary Provisioning feature is enabled.") | ||
|
|
||
| reportIssues := "If you encounter any unexpected user experience or something didn't go as planned," + |
There was a problem hiding this comment.
I think this message made more sense when we had the INFO message about binary provisioning enable. Now I don't see we should use it. See my comments on the error messages.
pablochacin
reviewed
Apr 24, 2025
internal/cmd/launcher.go
Outdated
| Error("Binary provisioning is enabled but it failed to analyze the dependencies." + | ||
| " Please, make sure to report this issue by opening a bug report.") | ||
| Error("Binary Provisioning is enabled but it failed to analyze the dependencies. " + | ||
| reportIssues) |
There was a problem hiding this comment.
Seme problem here with reusing reportIssues
Suggested change
| reportIssues) | |
| " Please, make sure to report this issue by opening a bug report.") |
pablochacin
reviewed
Apr 24, 2025
pablochacin
approved these changes
Apr 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Changes to improve the UX.
Why?
There is also a change that skips the launcher very early in the root code. As we are still early days, it might be better if we don't take a risk disrupting the stable execution path.
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)