add extension version #2754

HarrisChu · 2022-11-02T07:52:40Z

I write a k6 extension and build with xk6, but I cannot know the extension version via command line.

if extension module implement VersionGetter interface, could display via version command.

package k6plugin

import (
	"github.com/vesoft-inc/k6-plugin/pkg/aggcsv"
	"github.com/vesoft-inc/k6-plugin/pkg/nebulagraph"
	"go.k6.io/k6/js/modules"
	"go.k6.io/k6/output"
)

func init() {
	modules.Register("k6/x/nebulagraph", nebulagraph.NewNebulaGraph())
	output.RegisterExtension("aggcsv", func(p output.Params) (output.Output, error) {
		return aggcsv.New(p)
	})
}

> ./k6 version
k6 v0.40.0 ((devel), go1.18.5, linux/amd64)
extension k6/x/nebulagraph v1.0.2

CLAassistant · 2022-11-02T07:52:46Z

All committers have signed the CLA.

codebien · 2022-11-02T08:17:09Z

Hey @HarrisChu,
thanks for your contribution 🙇

Please, check this previous comment where we shared our vision regarding this feature.

HarrisChu · 2022-11-02T09:53:51Z

perfect for me.
so I should close this pr?

codebien · 2022-11-02T10:13:03Z

If you would like to contribute then you can keep this open and address it as a request change.

HarrisChu · 2022-11-03T01:40:16Z

I cannot test the code as debug.ReadBuildInfo not populated for test
just test it with my extension.

imiric

Thanks for this PR @HarrisChu! 🙇

We've had #1741 in our backlog for a long time, so it's great that it's finally being implemented. As you've found, it's an important feature for k6 extension maintainers.

I do have a couple of requirements before merging this. Namely making the k6 run output more pleasing, and also showing the versions of any output extensions. The others are suggestions that would be nice to have, but not blockers.

Since testing this is a bit tricky, we might want to have an E2E test instead. Currently we have an xk6 CI workflow, so it might make sense to add an assertion or two about the actual output of both k6 run and k6 version there. Unless someone has a better idea to test this...

imiric · 2022-11-09T17:48:50Z

cmd/ui.go

-	return c.Sprint(consts.Banner())
+	banner := make([]string, 0, len(modules.GetJSModuleVersions())+1)
+
+	banner = append(banner, consts.Banner())
+	for pkg, version := range modules.GetJSModuleVersions() {
+		banner = append(banner, fmt.Sprintf("extension  %s %s", pkg, version))
+	}
+	return c.Sprint(strings.Join(banner, "\n"))


I think this output needs to be a bit more visually pleasing. Here's what it currently looks like:

The extension information shouldn't be part of the banner. The banner itself is only reserved for... well, the k6.io banner 😄

Instead, if we do want to include this information as part of the k6 run output in addition to the k6 version output (I do think it makes sense to have it in both places), then let's integrate it into the section below the banner, as suggested in #1741.

imiric · 2022-11-09T18:14:40Z

cmd/version.go

+			for path, version := range modules.GetJSModuleVersions() {
+				printToStdout(globalState, fmt.Sprintf("extension %s %s\n", path, version))
+			}


k6 supports both JS and output extensions. So we should make sure that both are shown in this output (same goes for the k6 run output).

You can test by building a binary with an output extension, e.g.:

XK6_K6_REPO=github.com/HarrisChu/k6 xk6 build master \ --with github.com/vesoft-inc/k6-plugin \ --with github.com/grafana/xk6-output-influxdb

It should be a matter of doing something similar as in js/modules/modules.go and calling getPackageVersion(mod) from output.RegisterExtension(). You'll probably want to move getPackageVersion() somewhere where it's accessible by both (maybe lib/extensions.go?).

imiric · 2022-11-09T18:24:25Z

js/modules/modules.go

+	modules        = make(map[string]interface{})
+	moduleVersions = make(map[string]string)


Would it make sense to reuse the modules map, and have its values be a small struct like:

type module struct { mod interface{} version string }

This way getPackageVersion() would only return the version, as its name implies, and not also set (another) global map value.

This will complicate things slightly, as you'll need to change wherever GetJSModules() is called (or change GetJSModules() itself to continue to return just the modules without the version; though I think the first approach is slightly better), but it should avoid the repetition, and be a bit cleaner in the end. It would also avoid the need for a separate GetJSModuleVersions() function.

imiric · 2022-11-09T18:26:23Z

js/modules/modules.go

+	getPackageVersion(mod)
+}
+
+func getPackageVersion(mod interface{}) {


Suggested change

func getPackageVersion(mod interface{}) {

func getModuleVersion(mod interface{}) {

I think this is slightly clearer, as we are getting the Go module version after all (and the map is called moduleVersions).

good catch.

btw, If a package has two modules, it just has one package in map.
how about display the module name? e.g.

extensions: github.com/dgzlopes/k6-extension-notifications k6/x/ext1 v1.0.0 github.com/dgzlopes/k6-extension-notifications k6/x/ext2 v1.0.0

codecov-commenter · 2022-11-10T10:06:58Z

Codecov Report

Merging #2754 (5dac238) into master (7f823f0) will decrease coverage by 0.03%.
The diff coverage is 85.84%.

❗ Current head 5dac238 differs from pull request most recent head d4addec. Consider uploading reports for the commit d4addec to get more accurate results

@@            Coverage Diff             @@
##           master    #2754      +/-   ##
==========================================
- Coverage   75.59%   75.55%   -0.04%     
==========================================
  Files         202      205       +3     
  Lines       15992    16427     +435     
==========================================
+ Hits        12089    12412     +323     
- Misses       3151     3244      +93     
- Partials      752      771      +19

Flag	Coverage Δ
ubuntu	`75.55% <85.84%> (+0.16%)`	⬆️
windows	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/common/context.go	`100.00% <ø> (ø)`
api/server.go	`71.42% <ø> (ø)`
api/v1/client/client.go	`0.00% <ø> (ø)`
api/v1/client/metrics.go	`0.00% <ø> (ø)`
api/v1/client/status.go	`0.00% <ø> (ø)`
api/v1/errors.go	`76.92% <ø> (ø)`
api/v1/group.go	`54.05% <ø> (ø)`
api/v1/group_jsonapi.go	`95.74% <ø> (ø)`
api/v1/group_routes.go	`60.86% <ø> (ø)`
api/v1/metric.go	`77.27% <ø> (+9.09%)`	⬆️
... and 248 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

imiric · 2022-11-22T15:59:47Z

Hi @HarrisChu, are you available to finish the work on the pending items? We'd like to release this in the upcoming v0.42.0, so this should be merged in the next week or two.

If not, that's OK as well, and someone from our side would pick it up. You'd still get credit in the release notes, of course 🙂

oleiade

I align with the comments and recommendations of @imiric 🙏🏻
Really good work though, looking forward to see those comments addressed 👍🏻

Co-authored-by: HarrisChu <[email protected]> This adds some structure and extracts common functionality for registering and retrieving extension information into a standalone package. Partly based on the work and feedback in #2754.

imiric · 2022-12-06T16:49:40Z

Closing this since it was superseded by #2805. Thanks again for your contribution @HarrisChu! 🙇

Co-authored-by: HarrisChu <[email protected]> This adds some structure and extracts common functionality for registering and retrieving extension information into a standalone package. Partly based on the work and feedback in #2754.

HarrisChu · 2023-01-12T06:31:00Z

sorry, I had some emergencies...
glad it's already in v0.42, I would update our plugins.

imiric · 2023-01-12T08:49:30Z

@HarrisChu No problem.

It didn't make it in v0.42.0, but it will be released in v0.43.0 a few weeks from now.

github-actions bot requested review from mstoykov and oleiade November 2, 2022 07:52

add extension version

d4addec

HarrisChu force-pushed the master branch from 4298c04 to d4addec Compare November 3, 2022 01:37

imiric self-requested a review November 9, 2022 12:31

imiric suggested changes Nov 9, 2022

View reviewed changes

imiric linked an issue Nov 9, 2022 that may be closed by this pull request

Display any extensions bundled by xk6 in the pre-execution output #1741

Closed

mstoykov added this to the v0.42.0 milestone Nov 15, 2022

oleiade requested changes Nov 23, 2022

View reviewed changes

na-- modified the milestones: v0.42.0, v0.43.0 Dec 2, 2022

imiric mentioned this pull request Dec 6, 2022

Show bundled extension versions in the k6 version output #2805

Merged

imiric closed this Dec 6, 2022

na-- removed this from the v0.43.0 milestone Dec 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add extension version #2754

add extension version #2754

HarrisChu commented Nov 2, 2022

CLAassistant commented Nov 2, 2022 •

edited

Loading

codebien commented Nov 2, 2022

HarrisChu commented Nov 2, 2022

codebien commented Nov 2, 2022

HarrisChu commented Nov 3, 2022

imiric left a comment

imiric Nov 9, 2022

HarrisChu Nov 10, 2022

imiric Nov 9, 2022

imiric Nov 9, 2022

HarrisChu Nov 10, 2022

imiric Nov 9, 2022

HarrisChu Nov 10, 2022 •

edited

Loading

codecov-commenter commented Nov 10, 2022

imiric commented Nov 22, 2022

oleiade left a comment

imiric commented Dec 6, 2022

HarrisChu commented Jan 12, 2023

imiric commented Jan 12, 2023

		modules = make(map[string]interface{})
		moduleVersions = make(map[string]string)

	func getPackageVersion(mod interface{}) {
	func getModuleVersion(mod interface{}) {

add extension version #2754

add extension version #2754

Conversation

HarrisChu commented Nov 2, 2022

CLAassistant commented Nov 2, 2022 • edited Loading

codebien commented Nov 2, 2022

HarrisChu commented Nov 2, 2022

codebien commented Nov 2, 2022

HarrisChu commented Nov 3, 2022

imiric left a comment

Choose a reason for hiding this comment

imiric Nov 9, 2022

Choose a reason for hiding this comment

HarrisChu Nov 10, 2022

Choose a reason for hiding this comment

imiric Nov 9, 2022

Choose a reason for hiding this comment

imiric Nov 9, 2022

Choose a reason for hiding this comment

HarrisChu Nov 10, 2022

Choose a reason for hiding this comment

imiric Nov 9, 2022

Choose a reason for hiding this comment

HarrisChu Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Nov 10, 2022

Codecov Report

imiric commented Nov 22, 2022

oleiade left a comment

Choose a reason for hiding this comment

imiric commented Dec 6, 2022

HarrisChu commented Jan 12, 2023

imiric commented Jan 12, 2023

CLAassistant commented Nov 2, 2022 •

edited

Loading

HarrisChu Nov 10, 2022 •

edited

Loading