Show bundled extension versions in the k6 version output
#2805
Conversation
Codecov Report
@@ Coverage Diff @@
## master-next #2805 +/- ##
===============================================
- Coverage 76.13% 75.96% -0.18%
===============================================
Files 212 211 -1
Lines 16501 16559 +58
===============================================
+ Hits 12563 12579 +16
- Misses 3170 3205 +35
- Partials 768 775 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
I love that PR 😍
I left a couple of comments in PR and have one more general.
Although an extension output looks excellent, there are no questions for the k6 version.
But I feel it's not always relevant to have the same level of details (precisely version and repository) for the k6 run 🤔 I had an impression that we tend to keep it minimal.
|
Thanks for reviewing @olegbespalov! You raise a good point regarding Since extension versions and maybe even the package path is not relevant for ? I kind of don't even want to split them over multiple lines, but if we show them on the same line we'd have to make sure to wrap properly if many extensions are in use or the terminal is too narrow. I can look into it if everyone else agrees. Let me know. |
There was a problem hiding this comment.
Really great work @imiric 👍🏻 🎉 🦖
I'm also super happy to see this happen! I left a couple of nitpicks for your consideration, but I wouldn't consider any of them blocking in any way. Personally, I don't necessarily mind the k6 run output as it is now 🙇🏻
|
|
||
| // All supported k6 extension types. | ||
| const ( | ||
| JSExtension ExtensionType = iota + 1 |
There was a problem hiding this comment.
That looks like an opportunity to learn something: why the +1 on the iota here? 😇
There was a problem hiding this comment.
Just a convention to avoid constants with a value of 0. That way you can identify when a value was set or not.
ext/ext.go
Outdated
| case JSExtension: | ||
| s = "js" | ||
| case OutputExtension: | ||
| s = "out" |
There was a problem hiding this comment.
UX nit: I would find "output" to be more explicit, especially if we show this to the user. "out" is quite a polysemic word, used in a ton of different contexts, and as a user, I think I could be confused/unclear by what it refers to.
There was a problem hiding this comment.
"output" is definitely clearer, but I wanted to keep this short, since it's shown in k6 run which is already loaded with information, especially if we decide to show it in a single line. Considering the context of this is "extensions", I doubt there would be confusion about what "out" means, especially if shown next to "js" (which I also want to keep short).
But OK, I renamed it in b975746.
| // Extension is a generic container for any k6 extension. | ||
| type Extension struct { | ||
| Name, Path, Version string | ||
| Type ExtensionType |
There was a problem hiding this comment.
Another very personal nit you should feel free to dismiss: would you consider renaming the field and type to Kind ExtensionKind. I find it to be some somewhat more accurate in that context, and it avoids using a name that's a language keyword. But I want to acknowledge this is a very personal thing 🙇🏻
There was a problem hiding this comment.
🤔 I think we've always referred to these as extension "types", so I wouldn't want to change our vocabulary just because it's a reserved keyword. Though in this case type is the keyword, and Type works just fine, so I'd rather keep calling it this, if you don't mind.
| type Extension struct { | ||
| Name, Path, Version string | ||
| Type ExtensionType | ||
| Module interface{} |
There was a problem hiding this comment.
This comment might be caused by the fact that I'm only partially through the PR, but I must admit that looking at this field's definition and how it's used in Register as a user, I would be unclear as to what kind of value I should use to set the Module field.
There was a problem hiding this comment.
Sure, it is confusing. 😞 We initially called this "module" when JS extensions were introduced, as JS extensions would register a struct of their JS module. But then we changed the API for output extensions and had them register a constructor function that would return a struct value, for initialization purposes. In retrospect, I think this was the wrong API to introduce, but since I doubt we'll change this, every extension type is now free to choose what works best.
Since I can't think of a more generic name that would fit better, I'm inclined to leave it as Module, but let me know if you have a better idea.
There was a problem hiding this comment.
Changes look good to me 👍
Approving, but ideally, before merging we should check with the broader team if #2805 (comment) makes sense or my concerns are not relevant, and we can go with the current implementations 👍
|
My vote would be to show extension information only in
|
na--
changed the title
k6 run and k6 version outputsk6 version output
This is based on the work done in #2754 (thanks @HarrisChu!), and the suggested changes there.
It evolved into something slightly more elaborate, where the bulk of the changes are to unify the extension registry, so that registration and retrieval of extension information is done in a single place. See commit cf5fa61 for details. We still need to keep the existing registration functions in
js/modules/andoutput/to avoid breaking extensions, but internally they're both now handled in the new rootextpackage. This DRYs the handling of extensions, and simplifies adding new types of extensions. It also simplifies the retrieval of extension versions, which is why it was done on top of #2754.Here's how it currently looks:
edit by @na-- : after some internal discussions, we removed the changes to
k6 run, for now.Closes #1741