-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Clone Bundle's URLs into lib.Archive #4532
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Nice catch @joanlopez 🎉 |
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 🚀 |
What?
It modifies the
Bundle.makeArchive
function soBundle
's URLs (pointers) aren't directly reused by thelib.Archive
it constructs, but cloned ("deep copy") and set.Why?
Because the
Archive.Write
method 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
Archive
s 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