Skip to content
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

hack: add script to sync common dependiences with core to the version core uses #335

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

roobre
Copy link
Collaborator

@roobre roobre commented Sep 7, 2023

Description

Dependencies getting out of sync can be problematic given that xk6-disruptor is typically built together with k6 core. We've always anticipated goja to be a potential dependency to watch for, but as #330 proves, others may also cause problems as well.

This PR adds a (go) script that looks for overlapping dependencies with k6 core which do not match the version used in core. Those dependencies are logged to stderr, and go get commands are generated to stdout to download the version k6 core is using.

That way, by running go run ./hack/depsync > /dev/null, one can easily get a list of unsynced dependencies. By running go run ./hack/depsync | bash, dependencies are synced to core.

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make test) and all tests pass.
  • I have run relevant e2e test locally (make e2e-xxx for agent, disruptors, kubernetes or cluster related changes)
  • Any dependent changes have been merged and published in downstream modules

@roobre roobre marked this pull request as ready for review September 7, 2023 16:04
@roobre
Copy link
Collaborator Author

roobre commented Sep 12, 2023

Running this right now breaks builds as the cobra downgrade removes a method we currently use.

As a workaround, go run ./hack/depsync | grep -ve cobra | bash should work fine.

@pablochacin
Copy link
Collaborator

Running this right now breaks builds as the cobra downgrade removes a method we currently use

@roobre Could you please elaborate on which cobra downgrade you refer to?

@roobre
Copy link
Collaborator Author

roobre commented Sep 12, 2023

@pablochacin Sure. k6 core is using cobra v1.4.0:

13:08:00 ~/Devel/xk6-disruptor $> go run ./hack/depsync > /dev/null
2023/09/12 13:08:11 detected k6 core version v0.46.0
2023/09/12 13:08:11 Mismatched versions for github.com/spf13/cobra:
Ours:v1.5.0
Core:v1.4.0

If I attempt to build with cobra v1.4.0, I get the following error:

13:09:16 ~/Devel/xk6-disruptor $> make
go test -race  ./...
# github.com/grafana/xk6-disruptor/cmd/agent/commands
cmd/agent/commands/root.go:41:8: c.cmd.SetContext undefined (type *cobra.Command has no field or method SetContext)

This makes sense as adding Command.SetContext is mentioned in the v1.5.0 changelog: https://github.com/spf13/cobra/releases/tag/v1.5.0

This is currently being used only in one place:

c.cmd.SetContext(ctx)
return c.cmd.Execute()
}

From what I can see from the cobra code/docs, it should be fairly easy to change those lines to:

return c.cmd.ExecuteContext(ctx)

Which should work in v1.4.0

@pablochacin
Copy link
Collaborator

Thanks for the explanation. I suggest opening a followup PR for using ExecuteContext instead of SetContext in our code to maintain compatibility with k6 without any workaround.

Copy link
Collaborator

@pablochacin pablochacin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great work.

@roobre roobre merged commit e803190 into main Sep 12, 2023
7 checks passed
@roobre roobre deleted the hack-sync-deps branch September 12, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants