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

Moving to a goja fork #3773

Closed
mstoykov opened this issue Jun 6, 2024 · 10 comments
Closed

Moving to a goja fork #3773

mstoykov opened this issue Jun 6, 2024 · 10 comments
Assignees
Labels

Comments

@mstoykov
Copy link
Contributor

mstoykov commented Jun 6, 2024

What?

This is an issue for the actual work that needs to be done around #3772.

Problem

Due to the fact we are doing a fork and how golang modules work we need to somehow tell the go toolchain that it now needs to use the goja fork from this place.

Solutions:

fork and use replace

First this means we need to keep it called goja and it means anyone else using this fork will have to forever use replace, unless we break again and move to one of the below methods.

Due to how xk6 works it also means that we will need to teach it to add replace for goja and figure out the correct version. Which also means that in order for people to continue using xk6 they need to know they have to upgrade it.

The only upside of this option is that we don't need to change much code.

At least until we do any visible changes to the API (new functions, types, changing some of them) then every extension also needs to put replace clause in its repo as well.

Given the above while it seems like the "easiest" option it will continue to haunt us forever. So I personally think this is a bad idea.

fork and switch

The obvious thing is to name the new module something and then go over the code and change it to use that new module.

This would break every extension the moment it has to be compiled with this new k6 - and they will need to the exact same thing.

The change is arguably fairly easy and can be done with regex maybe gopatch for some parts.

We can also make PRs for every extension as we have done some times before

Unfortunately due to how experimental modules are implemented the original PR for this within k6 will be quite the nightmare and will need to happen for all extensions. This potentially might block people working on other work and likely will be quite hard to revert if we end up needing to pause the whole thing.

As such if we are going to be doing this I would recommend doing it early in the release.

alias, switch and then fork

The alternative is to make a new module with the new name and instead of putting the goja code in - we alias every type and copy-paste the consts and variables.

This way you can now point k6 code to this new module - it will work. But at the same time every extension will be able to build as for go those are the same types and the functions and variables and consts get matched by values.

This than lets us having a much smaller initial PR for k6, go through all the experimental modules within the next days/weeks, update them to the new module and update them in k6.

We can do the same for extensions, and they will work with both old k6 but also the version of k6 we release with this.

After 1 release of k6 we make new version of the module now with the actual goja code - actually forking it. At this point updating it in k6 will just work and as long as extensions have actually updated - it will also work for them.

AFAIK there is no problem with this idea, and we only need to do it for the goja package itself not for any of the other ones in the module - as only k6 cares about parser and ast

In practice the amount of work here is more or less the same - I already have the code for the alias - took me some regexing and experimenting - probably an hour in total. And have "dry" tested the idea within a go workplace.

The major upside is that we don't need to do all of these changes all at once. This IMO will make the changes considerably easier to actually do without that much cross team coordination.

The downside is it having more steps and potentially adding some confusion in the period of transition.

If we agree to this we can start doing it this release and fork at the beginning of the next one.

@inancgumus
Copy link
Member

inancgumus commented Jun 6, 2024

Is my understanding correct for the third solution?

First step:

module (e.g., browser) -> "new module" aliases to -> goja (temporarily)

Second step:

module (e.g., browser) -> "new module" is a fork of goja

It sounds to me like a sound idea if it's like so.

✅ Update: I vote for the third solution: alias, switch and then fork

@github-actions github-actions bot added the triage label Jun 6, 2024
@mstoykov mstoykov self-assigned this Jun 6, 2024
@mstoykov mstoykov removed the triage label Jun 6, 2024
@mstoykov
Copy link
Contributor Author

mstoykov commented Jun 6, 2024

@inancgumus yeah

@mstoykov
Copy link
Contributor Author

mstoykov commented Jun 6, 2024

For the record the problem with experimental modules in the second option is that we need to:

  1. remove the experimental modules(at least the ones that are external).
  2. then do the changes in k6
  3. then update the experimental modules code to the newly updated k6 + goja changes
  4. readd them in k6

As in this case we did only for xk6-websockets.

As the change is way bigger it will practically block any development as we need to "sync" every experimental module and k6.

@olegbespalov
Copy link
Contributor

"alias, switch and then fork" sounds like the best option 👍

@ankur22
Copy link
Contributor

ankur22 commented Jun 6, 2024

@mstoykov the main worry is what happens to the other extensions, not necessarily the experimental extensions since we can work on the changes ourselves, correct?

If we went with proposal 2, it would mean that all extensions that we don't maintain will no longer build with xk6, and therefore we would force those maintainers to switch over to the new fork of goja first before being able to build with xk6, correct?

Proposal 3 allows k6 to move to an alias module, while allowing all extensions to build in the short term, giving all the extension maintainers time to switchover to the new alias module, before finally switching k6 over to the actual fork. If maintainers didn't switch over to the alias module, what would happen to their extension once k6 does switch over to the fork of goja? They wouldn't build, correct?

So the outcome of both proposal 2 and 3 for extensions we do not maintain could be the same if the maintainers do not action on the changes, but in proposal 3 we're giving them more time to switch over?

@mstoykov
Copy link
Contributor Author

mstoykov commented Jun 6, 2024

@ankur22 yes, I probably should've specified this more in the original post.

With 2. - we make the change and after that every version of k6 will not work with not update extensions, from the moment we merge it (if people use master) or if they use latest from when we release the version after that.

Because xk6 doesn't do this automatically - extensions will just compile with the k6 version they specify. That changes actually is specifically there because otherwise any breaking change always broke every extension with 0 time for them to react.

With 3. - extensions that aren't updated will work with k6 versions before the aliases and then until the aliases work. If an extension developer updates to k6 version with the alias that will conitnue to work forward. Technically it will also work "backwards" if the user uses 2 extensions and one isn't updated - but the second extension will break the moment we fork it.

So yes option 3 does give extension developers everywhere the time between us releasing a k6 version with alias and moving to a fork, as time to actually update. This likely will be a single release, so unfortunately a lot smaller than we would probably want for a change like this, but still likely way better than nothing.

mstoykov added a commit that referenced this issue Jun 6, 2024
This is part of #3772 and #3773.

Moving to the currently only aliases codebase lets us update dependent
modules such as extension one at a time.
mstoykov added a commit that referenced this issue Jun 6, 2024
This is part of #3772 and #3773.

Moving to the currently only aliases codebase lets us update dependent
modules such as extension one at a time.
@joanlopez
Copy link
Contributor

Count another vote in favor of "alias, switch and then fork" from my side 👍

mstoykov added a commit that referenced this issue Jun 6, 2024
This is part of #3772 and #3773.

Moving to the currently only aliases codebase lets us update dependent
modules such as extension one at a time.
mstoykov added a commit to grafana/xk6-browser that referenced this issue Jun 6, 2024
We are moving to a fork of goja under grafana org called serbo.

More info in:
- grafana/k6#3772
- grafana/k6#3773
mstoykov added a commit to grafana/xk6-dashboard that referenced this issue Jun 6, 2024
We are moving to a fork of goja under grafana org called serbo.

More info in:
- grafana/k6#3772
- grafana/k6#3773
mstoykov added a commit to grafana/xk6-output-prometheus-remote that referenced this issue Jun 6, 2024
We are moving to a fork of goja under grafana org called serbo.

More info in:
- grafana/k6#3772
- grafana/k6#3773
mstoykov added a commit to grafana/xk6-redis that referenced this issue Jun 6, 2024
We are moving to a fork of goja under grafana org called serbo.

More info in:
- grafana/k6#3772
- grafana/k6#3773
mstoykov added a commit to grafana/xk6-webcrypto that referenced this issue Jun 6, 2024
We are moving to a fork of goja under grafana org called serbo.

More info in:
- grafana/k6#3772
- grafana/k6#3773
mstoykov added a commit to grafana/xk6-websockets that referenced this issue Jun 6, 2024
We are moving to a fork of goja under grafana org called serbo.

More info in:
- grafana/k6#3772
- grafana/k6#3773
@szkiba
Copy link
Contributor

szkiba commented Jun 6, 2024

The FieldNameMapper functions should also have an alias...
ie: UncapFieldNameMapper

@mstoykov
Copy link
Contributor Author

mstoykov commented Jun 6, 2024

@szkiba I specifically didn't add this as it isn't a thing any extension should use with the k6 runtime.

Your use case in the xk6-dashboard seems okay - so after I polish the rest of the process I will probably add it just for it.

mostafa pushed a commit to mostafa/xk6-kafka that referenced this issue Jun 10, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
acuenca-facephi pushed a commit to acuenca-facephi/xk6-read that referenced this issue Jun 10, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
nicholasvuono pushed a commit to nicholasvuono/xk6-playwright that referenced this issue Jun 10, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
JorTurFer pushed a commit to JorTurFer/xk6-input-prometheus that referenced this issue Jun 10, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
mstoykov added a commit that referenced this issue Jun 11, 2024
There are still some leftover comments and names referencing goja that
need to be updated, but this compiles.

This closes #3772 and closes #3773
leonyork pushed a commit to leonyork/xk6-output-timestream that referenced this issue Jun 11, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
mstoykov added a commit to grafana/xk6-output-influxdb that referenced this issue Jun 11, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
jt-shippit pushed a commit to jt-shippit/xk6-sql that referenced this issue Jun 14, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
@mstoykov
Copy link
Contributor Author

All of the PRs linking back to here were created with a script that can be find here

The relevant parts for the transition are:

go mod edit -require go.k6.io/k6@1f01a9bc23653d169f16ef70e935b263e80f1a90 && go mod tidy -go=1.20
find . -name "*.go" -type f -exec sed -i -e 's|goja\.|sobek\.|g' {} \;
find . -name "*.go" -type f -exec sed -i -e 's|"github.com/dop251/goja"|"github.com/grafana/sobek"|g' {} \;

As those update k6 up to it using sobek and then replace imports.

The remaining part is reformatting some other cleanup and the needed stuff to checkout code and create PRs.

mstoykov added a commit to grafana/xk6-loki that referenced this issue Jun 21, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
mstoykov added a commit to grafana/xk6-client-prometheus-remote that referenced this issue Jun 21, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
mstoykov added a commit that referenced this issue Jun 21, 2024
There are still some leftover comments and names referencing goja that
need to be updated, but this compiles.

This closes #3772 and closes #3773
skryukov pushed a commit to anycable/xk6-cable that referenced this issue Jun 24, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
mstoykov added a commit that referenced this issue Jun 26, 2024
There are still some leftover comments and names referencing goja that
need to be updated, but this compiles.

This closes #3772 and closes #3773
thmshmm pushed a commit to thmshmm/xk6-opentelemetry that referenced this issue Jun 26, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
pmalhaire pushed a commit to pmalhaire/xk6-mqtt that referenced this issue Jul 9, 2024
We are moving to a fork of goja under grafana org called sobek.

More info in:
- grafana/k6#3772
- grafana/k6#3773
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants