8000 Move not public parts of the k6 API in internal package by mstoykov · Pull Request #4133 · grafana/k6 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Move not public parts of the k6 API in internal package #4133

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 22, 2025
Merged

Move not public parts of the k6 API in internal package #4133

merged 2 commits into from
Jan 22, 2025

Conversation

mstoykov
Copy link
Contributor
@mstoykov mstoykov commented Dec 19, 2024 •
edited
Loading

What?

Move a lot of k6 code under internal .

This is done through some scripts that are also committed and will be removed after the final move.

Why?

Define k6 public API by moving everything else in internal.

This is hopefully the first in multiple steps. This is the one that is the most brutforce and moves the most amount of code around.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

@mstoykov mstoykov changed the base branch from master to websocketsMerge December 19, 2024 10:25
@mstoykov mstoykov self-assigned this Jan 15, 2025
@mstoykov mstoykov changed the title Internalize Move not public parts of the k6 API in internal package Jan 15, 2025
Base automatically changed from websocketsMerge to master January 15, 2025 14:53
@mstoykov mstoykov force-pushed the internalize branch 3 times, most recently from 223ec99 to 92cd119 Compare January 16, 2025 15:20
@mstoykov mstoykov mentioned this pull request Jan 16, 2025
Merged
5 tasks
@mstoykov mstoykov added this to the v0.57.0 milestone Jan 17, 2025
@mstoykov mstoykov force-pushed the internalize branch 2 times, most recently from 81c1a61 to a193a0f Compare January 20, 2025 11:20
@mstoykov mstoykov mentioned this pull request Jan 21, 2025
Merged
5 tasks
@mstoykov mstoykov marked this pull request as ready for review January 21, 2025 15:14
@mstoykov mstoykov requested a review from a team as a code owner January 21, 2025 15:14
@mstoykov mstoykov requested review from ankur22 and codebien and removed request for a team January 21, 2025 15:14
@mstoykov
Copy link
Contributor Author

Please, any reviewer run the internalize.sh script locally after checking out the first commmit and see if it makes the same changes.

@mstoykov mstoykov force-pushed the internalize branch 4 times, most recently from 19bea63 to 5ee4772 Compare January 21, 2025 15:55
@codebien
Copy link
Contributor 
git restore -s master -- ./
./internalize.sh
git status

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	cloudapi/insights/
	js/modules/k6/browser/
	js/modules/k6/experimental/
	js/summary-wrapper.js
	js/summary.js
	js/tc39/
	lib/testutils/
	output/cloud/expv2/integration/
	output/cloud/expv2/pbcloud/
	vendor/github.com/grafana/xk6-dashboard/dashboard/testdata/
	vendor/github.com/grafana/xk6-webcrypto/
	vendor/github.com/grafana/xk6-websockets/
	vendor/go.opentelemetry.io/otel/sdk/metric/internal/exemplar/
	vendor/go.opentelemetry.io/otel/verify_examples.sh
	vendor/google.golang.org/protobuf/internal/errors/is_go112.go
	vendor/google.golang.org/protobuf/internal/errors/is_go113.go

Instead, I would expect an empty list here. Is it expected? 🤔 The reason should be a different starting point of master.

@codebien
Copy link
Contributor

@mstoykov We should also consider to lock master to prevent to rebase all the time. Can you rebase the last time and lock it, please?

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#lock-branch

@mstoykov
Copy link
Contributor Author

@codebien can you try again, please. Yeah unfortunately making changes to master makes this constantly move. And we did merge a bunch of stuff yesterday after I finally managed to get most of the final things doen.

On locking master - I kind of prefer that I do not block everyone from being able to merge stuff, given this takes me 2-5 minutes to rebase.

@codebien
Copy link
Contributor
codebien commented Jan 22, 2025 •
edited
Loading 
git fetch origin
git reset --hard origin/internalize
git switch master
git pull --rebase origin master
git switch internalize
git restore -s master -- ./
git restore internalize.patch internalize.sh publicly_used_imports.txt
./internalize.sh

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	cloudapi/insights/
	js/modules/k6/browser/
	js/modules/k6/experimental/
	js/modules/k6/webcrypto/
	js/summary-wrapper.js
	js/summary.js
	js/tc39/
	lib/testutils/
	output/cloud/expv2/integration/
	output/cloud/expv2/pbcloud/

@mstoykov mstoykov merged commit 7129195 into master Jan 22, 2025
29 checks passed
@mstoykov mstoykov deleted the internalize branch January 22, 2025 15:39
@mstoykov mstoykov mentioned this pull request Jan 22, 2025
Merged
5 tasks
@olegbespalov olegbespalov mentioned this pull request Jan 29, 2025
Merged
8 tasks
@BrewTestBot BrewTestBot mentioned this pull request Feb 13, 2025
Merged
@kempsterc kempsterc mentioned this pull request Mar 3, 2025
Closed
This was referenced Mar 14, 2025
Closed
Merged
mstoykov added a commit that referenced this pull request Apr 8, 2025
@mstoykov
Move CSV output to internal as preparations for v1.0.0
049043a 
This was not moved before as a utility function `IsStringInSlice` used
by a now not maintained extension:
- https://github.com/Maksimall89/xk6-output-clickhouse

As that dependency doesn't make any sense it was planned to be moved
either way. But as the extension is now not maintained and has not merged
previous PRs to fix breaking changes, it is even more of a reason to
move forward.

Related to #4133
@mstoykov mstoykov mentioned this pull request Apr 8, 2025
Merged
8 tasks
mstoykov added a commit that referenced this pull request Apr 8, 2025
@mstoykov
Move k6 js module to internal as preparations for v1.0.0
5360c4d 
This wasn't previously done due to the exported `ErrCheckInInitContext`
being used in a couple of extensions that should likely come up with
their own errors.

Related to #4133
@mstoykov mstoykov mentioned this pull request Apr 8, 2025
Merged
8 tasks
mstoykov added a commit that referenced this pull request Apr 9, 2025
@mstoykov
Move CSV output to internal as preparations for v1.0.0
f24c43f 
This was not moved before as a utility function `IsStringInSlice` used
by a now not maintained extension:
- https://github.com/Maksimall89/xk6-output-clickhouse

As that dependency doesn't make any sense it was planned to be moved
either way. But as the extension is now not maintained and has not merged
previous PRs to fix breaking changes, it is even more of a reason to
move forward.

Related to #4133
mstoykov added a commit that referenced this pull request Apr 9, 2025
@mstoykov
Move k6 js module to internal as preparations for v1.0.0
aa1e289 
This wasn't previously done due to the exported `ErrCheckInInitContext`
being used in a couple of extensions that should likely come up with
their own errors.

Related to #4133
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankur22 ankur22 ankur22 approved these changes

@codebien codebien codebien approved these changes

Assignees

@mstoykov mstoykov

Labels
None yet
Projects
None yet
Milestone
v0.57.0
Development

Successfully merging this pull request may close these issues.
3 participants
@mstoykov @codebien @ankur22
0