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.

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

Deprecate context-based utils #2384

Merged
merged 2 commits into from
Feb 11, 2022
Merged

Deprecate context-based utils #2384

merged 2 commits into from
Feb 11, 2022

Conversation

codebien
Copy link
Contributor

@codebien codebien commented Feb 10, 2022 •
edited
Loading

Deprecating the context-based utils functions. They are anticipating the next step where we could remove all of them from the k6 public API.

Closes #2344

@codebien codebien added this to the v0.37.0 milestone Feb 10, 2022
@codebien codebien self-assigned this Feb 10, 2022
codebien
codebien commented Feb 10, 2022
View reviewed changes
lib/context.go Outdated
Comment on lines 57 to 58
state.Logger.Warn("GetState has been deprecated and it could be removed in the next versions." +
"Please, consider to replace it using the new modules.VU API.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we use a sync.Once for each method for avoiding printing the warning multiple times?

Copy link
Member

Choose a reason for hiding this comment

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

Probably a good idea 🤔

codebien
codebien commented Feb 10, 2022
View reviewed changes
js/common/context.go Outdated
func GetInitEnv(ctx context.Context) *InitEnvironment {
logger := logger(ctx)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we check and skip in case of logger == nil? It would be a bug but maybe avoid us to hotfix it

na--
na-- reviewed Feb 10, 2022
View reviewed changes
js/bundle.go Outdated Show resolved Hide resolved
@na-- na-- requested a review from mstoykov February 10, 2022 14:45
@codebien codebien force-pushed the deprecate-ctx-utils branch 2 times, most recently from 3e9c0b8 to ec7357c Compare February 10, 2022 17:48
@codebien codebien requested a review from na-- February 10, 2022 18:47
@codebien
Copy link
Contributor Author

I removed all the logs stuff and just added what suggested by #2384 (comment)

olegbespalov
olegbespalov previously approved these changes Feb 10, 2022
View reviewed changes
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

LGTM 👍

mstoykov
mstoykov requested changes Feb 10, 2022
View reviewed changes
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

I am pretty sure that the log warning will be printed per each VU(but haven't tested). It's probably good idea if it's only once though ;).

Although on the other hand,it will probably drive the point home that this requires changes if it fills the screen 🤣

js/initcontext.go Outdated
Comment on lines 217 to 224
i.logger.Warnf(`Module '%s' is using deprecated APIs that will be removed in k6 v0.38.0,`+
` for more details on how to update it see`+
` https://k6.io/docs/extensions/guides/create-an-extension/#advanced-javascript-extension`, name)

return i.moduleVUImpl.runtime.ToValue(common.Bind(i.moduleVUImpl.runtime, mod, i.moduleVUImpl.ctxPtr)), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I am pretty sure this will be logged for each VU

js/initcontext.go Outdated
Comment on lines 214 to 215
mod = perInstance.NewModuleInstancePerVU()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should drop this lines and the modules.HasModuleInstancePerVU interface 🤔

mstoykov
mstoykov previously approved these changes Feb 11, 2022
View reviewed changes
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

This will print only once even if multiple modules don't use the new API. But I see this as corner case that isn't worth the complexity ;)

@codebien
Deprecated context-based utils
ff4610f
@codebien
js/modules: Drop HasModuleInstancePerVU interface
c9958ec 
The new modules API made this API not required anymore. Both internal
and external modules can now use the same unique way for accessing the VU State.
@codebien
Copy link
Contributor Author

This will print only once even if multiple modules don't use the new API. But I see this as corner case that isn't worth the complexity ;)

Yep, it was my motivation for printing on every call to require. But based on your previous comment I changed my opinion, the user can always fix run by run and it maintains the code simpler without adding and injecting dependencies, in the end, it will be there for only one release.

@na--
Copy link
Member

na-- commented Feb 11, 2022

I haven't tested it with an extension though, just read the code

@codebien codebien merged commit c4b88f5 into master Feb 11, 2022
@codebien codebien deleted the deprecate-ctx-utils branch February 11, 2022 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstoykov mstoykov mstoykov approved these changes

@na-- na-- na-- approved these changes

@olegbespalov olegbespalov Awaiting requested review from olegbespalov

Assignees

@codebien codebien

Labels
None yet
Projects
None yet
Milestone
v0.37.0
Development

Successfully merging this pull request may close these issues.

Deprecate common.Bind() and relatives
4 participants
@codebien @na-- @mstoykov @olegbespalov