-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Refactor lib/consts/consts.go #4537
Conversation
df82813
to
7e172af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
lib/consts/consts.go
Outdated
// | ||
// Deprecated: alias to support the legacy versioning API. Use the new version package, | ||
// it will be removed as soon as the external services stop to depend on it. | ||
const Version = version.SemVer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that migration to the new const is hard enough? I mean, why not clean the package and just announce the breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is the OTel output, which is part of the vendor folder. If I drop the alias, then k6 is not buildable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, but if the OTEL is the only issue, it's not so challenging to iterate on that either do to step migration of the OTEL or relaxing it from usage of the package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration is the plan, but for doing it, we need the new value available, this is the reason for the alias.
We merge this pull request then:
- We can open the pull request on OTel output to fix to migrate to the new version
- We release a new version
- We open a new pull request on k6 for the new OTel version
- We open a new pull request on k6 for dropping the alias
or relaxing it from usage of the package
I didn't check into it, I assumed the current form is the best because it fetches automatically from k6 without us updating the value. But maybe there is a better way.
What is your preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with both strategies 👍 So we can keep the PR as it's and use as an intermediate step for switching usage in OTEL output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another maybe alternative could be backmerging the OTEL to the k6, which also should solve OTEL dependency issues that happening from time to time. The output remains experimental, but its management and maintenance will be simpler and straightforward.
I'm also curious to hear @mstoykov opinion, since I feel he has one in that topic in his mind 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr 👍 on backmerging 1, not certain if the version we send should be
Part of the idea of the code changes was to move experimental extensions in k6.
As I've said before experimental modules and outputs are in separate repos because we started that way. Therewere extensions that we decided to add as experimental. They were already in separate repos and we just kept them there.
Arguably in some cases this lets us have faster development, but to be honest this can also be done by not exposing the not extension by default.
By diagrace - in all cases most of the code is already in the repo in the vendor folder.
Looking at the usage of this, I have two thoughts:
- probably there should be a way to get k6 version, both in js and go
- not certain opentelemtry should be sending k6 version. I do not know what this is used for, but
2.1. k6 version on an extension doesn't mean much abotu the extension version, so if it something about finding that you ran otel output with X version that did that, k6's version, isn't waht you want - you want the extension one
2.2. if it is is just a formality of otel that we need to have a version, I guess in that case it is okay
Footnotes
-
not certain on the term, givne that it was never in k6 and it isn't an upstream, but 🤷 ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not certain on the term, givne that it was never in k6 and it isn't an upstream, but 🤷 ↩
🤦 Right, it's entirely wrong term here, but I meant merging code of the OTEL output to the k6, same way how we have done with browser, xk6-webcrypto and recently Prometheus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I did get that meaning :)
7e172af
to
2cdc182
Compare
version/version.go
Outdated
|
||
// SemVer contains the current version of k6 | ||
// represented using Semantic Versioning expression. | ||
const SemVer = "0.56.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super convinced SemVer
is really the best choice here, is there any reason why not to keep Version
?
Do we really expect this package to contain multiple "formats" of the k6 version?
I'd like to see what other colleagues think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really expect this package to contain multiple "formats" of the k6 version?
Not really, but it still makes sense for me because it's what it contains. A version expressed in semantic versioning format. So the full name version.SemVer
is self-expressive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess one could argue that version.SemVer
may also express that there are other representations of the k6 version, like version.Full
(referred to as vX.Y.Z
for instance), which isn't the case.
Anyway, I don't see major problems going with this one, and we can always add a Version
if needed, preferred. So, once we reach consensus around the alias and the OTEL extension, I'm going to give it a 👍🏻
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I share the same feeling as @joanlopez, it was also one of the reasons why I raised the thread #4537 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joanlopez @olegbespalov Just to clarify, now that we clarified the inconvenience of k6.Version
what is the request here? To have version.Version
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this will not be needed to be public one as we move otel output inside k6 , can we just ove it to internal/lib/consts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about meta.Version (go.k6.io/k6/meta.Version)?
I'm fine with that, but I would find difficult to explain what it should contain. What is the difference with the current lib
package? There is the risk to add another junk drawer similar to lib
or common
, where I would find it difficult to understand if the code I might want to add in the future fits it or not.
For example, to stay concrete, can you provide a description that you would put as documentation for the meta
package?
By the way, I have three additional suggestions:
semver.Version
buildinfo.Version
appversion.Version
Do you prefer one of them? Probably, I prefer buildinfo
the most as it clearly fits the current purpose and let space for expansion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mstoykov Yes, we can do it, and it's a good idea if we merge OTel. But one of my original goals were also to use this pull request to find a better place for those constants instead to stay in a generic lib
package.
So, I think it still makes sense to have go.k6.io./k6/internal/buildinfo.Version
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe meta
is more descriptive than a utility-like package lib
. Another one could be metadata
. But, still, I agree that they can turn out to be grab-all. So, I also suggested using a build
package, which "provides" k6-build-related information, like version. This can also go into an internal package (go.k6.io/k6/internal/build.Version
- it reads well to me).
// Package build provides information about the k6 binary's build.
package build
// Version is the current semantic version of k6.
const Version = "0.57.0"
// ...add other build-related information when needed...
semver
and appversion
are too narrow. I'm also fine with buildinfo
, but I'm a fan of more concise names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've incorporated all the suggestions and I think we've reached a good balance.
9c304db
to
6ff7d74
Compare
6ff7d74
to
ed810b3
Compare
ff70808
to
4d9babe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
What?
It creates a dedicated package for version management. It moves the Semantic Versioning constant string to the new internal pkg.
Why?
It reduces the public surface of k6 API.