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

Add GetScenarioOptions method to ExecutorConfig interface #3053

Merged
merged 3 commits into from
May 9, 2023

Conversation

imiric
Copy link
Contributor

@imiric imiric commented May 3, 2023

This simplifies retrieval of ScenarioOptions (added in #3036), since it avoids type assertions of all possible ExecutorConfig implementations. See this comment.

I also added an archive test to serialize the scenario options and read them back, which was arguably missing from #3036.

@github-actions github-actions bot requested review from codebien and mstoykov May 3, 2023 11:41
@imiric imiric requested review from ka3de and codebien and removed request for codebien and mstoykov May 3, 2023 11:42
@codecov-commenter
Copy link

codecov-commenter commented May 3, 2023

Codecov Report

Merging #3053 (82f1b1a) into master (e190703) will increase coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 82f1b1a differs from pull request most recent head b4f1458. Consider uploading reports for the commit b4f1458 to get more accurate results

@@            Coverage Diff             @@
##           master    #3053      +/-   ##
==========================================
+ Coverage   77.01%   77.03%   +0.01%     
==========================================
  Files         229      229              
  Lines       17065    17066       +1     
==========================================
+ Hits        13143    13147       +4     
+ Misses       3080     3078       -2     
+ Partials      842      841       -1     
Flag Coverage Δ
ubuntu 77.03% <0.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
lib/executor/base_config.go 84.21% <0.00%> (-2.28%) ⬇️
lib/executors.go 93.10% <ø> (ø)

... and 4 files with indirect coverage changes

ka3de
ka3de previously approved these changes May 3, 2023
Copy link
Contributor

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

Thanks for this! LGTM.

imiric pushed a commit that referenced this pull request May 5, 2023
@imiric imiric force-pushed the feat/executorconfig-scenariooptions branch from 59d17da to 2d82729 Compare May 5, 2023 15:08
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for the green CI

@imiric
Copy link
Contributor Author

imiric commented May 5, 2023

Apparently there's an issue building gotip 😮‍💨

go: downloading golang.org/dl v0.0.0-20230502172222-5216546bad51
Cloning into '/home/runner/sdk/gotip'...
Updating the go development tree...
From https://go.googlesource.com/go
 * branch            master     -> FETCH_HEAD
HEAD is now at 761e813 cmd/compile: work with new bisect command
Building Go cmd/dist using /opt/hostedtoolcache/go/1.20.4/x64. (go1.20.4 linux/amd64)
Building Go toolchain1 using /opt/hostedtoolcache/go/1.20.4/x64.
# bootstrap/internal/abi
/home/runner/sdk/gotip/src/internal/abi/unsafestring_go120.go:13: unsafe.String requires go1.20 or later (-lang was set to go1.16; check go.mod)
/home/runner/sdk/gotip/src/internal/abi/unsafestring_go120.go:17: unsafe.Slice requires go1.17 or later (-lang was set to go1.16; check go.mod)
go tool dist: FAILED: /opt/hostedtoolcache/go/1.20.4/x64/bin/go install -tags=math_big_pure_go compiler_bootstrap purego bootstrap/cmd/...: exit status 1
gotip: failed to build go: exit status 2

Ivan Mirić added 2 commits May 9, 2023 10:32
This simplifies retrieval of ScenarioOptions, since it avoids type
assertions of all possible ExecutorConfig implementations.

See grafana/xk6-browser#858 (comment)
I'm not adding it to the lib package, since it could possibly result in
an import loop (lib -> lib/executor -> lib).
@imiric imiric force-pushed the feat/executorconfig-scenariooptions branch from 2d82729 to 35af258 Compare May 9, 2023 08:33
@imiric imiric requested a review from codebien May 9, 2023 10:42
codebien
codebien previously approved these changes May 9, 2023
@imiric
Copy link
Contributor Author

imiric commented May 9, 2023

@codebien Can you also review #3057, since you mentioned you want to merge that one before this?

Previously, the new scenario `options` field added in #3036 was being
serialized to JSON by default, which made it an incompatible change with
some of our internal Cloud services that validate the existing
configuration structure.

This change makes the object optional, and it will only be serialized if
it's included in the data.
@imiric imiric requested a review from ka3de May 9, 2023 13:54
Copy link
Contributor

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

LGTM.

@imiric imiric merged commit d951eac into master May 9, 2023
@imiric imiric deleted the feat/executorconfig-scenariooptions branch May 9, 2023 14:52
@mstoykov mstoykov added this to the v0.45.0 milestone Jun 5, 2023
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.

5 participants