Clone Bundle's URLs into lib.Archive #4532
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
joanlopez
commented
Feb 10, 2025
| @@ -164,13 +164,16 @@ func NewBundleFromArchive(piState *lib.TestPreInitState, arc *lib.Archive) (*Bun | |||
| } | |||
|
|
|||
| func (b *Bundle) makeArchive() *lib.Archive { | |||
| clonedSourceDataURL, _ := url.Parse(b.sourceData.URL.String()) | |||
| clonedPwdURL, _ := url.Parse(b.pwd.String()) | |||
|
|
|||
| arc := &lib.Archive{ | |||
| Type: "js", | |||
| Filesystems: b.filesystems, | |||
There was a problem hiding this comment.
Other fields here, like this Filesystems are also passed by reference (note it's a map), and thus are likely to be modified. However, that doesn't seem to cause any issue so far.
mstoykov
approved these changes
Feb 10, 2025
|
Nice catch @joanlopez 🎉 |
olegbespalov
approved these changes
Feb 10, 2025
|
Looks good! Would it be possible to introduce a test case for that (maybe in separated PR)? |
|
Just passing by to send a huge kudos for fixing this @joanlopez 🚀 |
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?
It modifies the
Bundle.makeArchivefunction soBundle's URLs (pointers) aren't directly reused by thelib.Archiveit constructs, but cloned ("deep copy") and set.Why?
Because the
Archive.Writemethod normalizes and anonymizes these URLs to prevent certain details (like OS's user, HOME dir, etc) to be leaked to the GCk6, but we not always want these modifications to be propagated to theBundle's URLs - e.g. when running the test script locally:k6 cloud ... --local-execution.Review notes
A better explanation of why this (#4168) happens is because the process goes as follows:
Note this started to fail since we introduced this change because the old way of doing the same was
k6 run -o cloud, and in that case we were not sending any archive at all. Now, the archive is an opt-out "feature", enabled by default.Also, note that another approach to fix this issue (#4168) could consist on something like "reinstantiating" the first runner, so the "new paths" are cached again, and/or to somehow make it all work with the normalized and anonymized URLs.
However, that sounded a bit counterintuitive to me, plus I suspected that may cause other issue later (like k6 errors/warning not pointing to the correct paths locally, etc).
While doing a "deep copy" of these URLs to "isolate" changes on
Archives sounded good to me, because I could not think of any scenario where that could be a problem, and logically it kinda makes sense to make theArchive"standalone".Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)
Fixes #4168