-
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
Add a design document proposal to support open in VU context #3358
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3358 +/- ##
=======================================
Coverage 72.96% 72.96%
=======================================
Files 261 261
Lines 20018 20018
=======================================
Hits 14607 14607
Misses 4484 4484
Partials 927 927
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 should add a comparison in terms of pros&cons for the solutions proposed and we should add a section for eventual risks.
I usually prefer coding soltuions but this time I would prefer the option. I think it enables the concept of injecting the infrastructure where the file system could be part of.
Having the option could allow us to control this injection from the CLI, which sounds closer to what ops are already doing, mounting volumes.
I have the feeling that the Option's implementation will be simpler as we don't have to go through the code and scan for this special function.
- Opening an un-included file in the VU stage will result in an error. | ||
Files opened in the init stage without prior inclusion should still operate as they currently do (though this is debatable) to maintain backward compatibility. | ||
|
||
#### Using dedicated option set |
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.
#### Using dedicated option set | |
### Solution B: A dedicated option set |
I evaluate it as a different proposal because they should be mutually exclusive. I don't see the reason why we should eventually implement both.
|
||
### Solution B: Awaiting Proposals | ||
|
||
Have a novel idea? Please share it with us in a dedicated commit :bow: |
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.
I would argue we do not need a new API at all.
There are two problems:
- you can't at all
open
files outside of init code - it might be beneficial to have an API that tells k6 to allow a given file to be openable outside of the init code.
There is also the sligth twist that actually open
will return you an error even in init code if you try to open a file in a VU after the initial one that was not opened by the initial one.
open("./someotherfile.txt") // will not error out for either the initial nor the VU with id `1`
if (__VU == 1) {
open("./somefile.txt") // will always error out as the initial VU did not open it.
}
Again the first open
is called both in VU with id 0
but also for any VU that will execute a default function (for example). See #1771 for the rational and discussion.
the tl;dr is that if we want to allow open
to at all execute in vu code we can have the same limitation - the file needs to be opened in the VU==0. This is one decision and change.
Adding API/configuration that adds files (again in the initial VU as discussed in #1771) is IMO a separate discussion.
We never prioritized to work on it. I'm removing myself as it is unrealistic that I will work soon on it. |
This draft aims to address #3020, focusing on enabling k6 scripts to access files directly from the VU stage, not just the init stage.
While the primary focus is on file access, I've currently left out considerations regarding
require
from #3020. If require should be part of this discussion, based on similar requirements or challenges, I'm more than open to updating the proposal to encompass it.If you've got alternative solutions or insights, please contribute with a separate commit.
I'm eager to hear your thoughts and feedback in the comments below. Thanks for taking the time to review! 🙇🏻