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

Lock module resolution after initialization #3680

Merged
merged 4 commits into from
Apr 22, 2024
Merged

Lock module resolution after initialization #3680

merged 4 commits into from
Apr 22, 2024

Conversation

mstoykov
Copy link
Collaborator

@mstoykov mstoykov commented Apr 8, 2024

What?

Make it an error to try to resolve not previously resolved modules, after initialization

Why?

Partly because it should be given how we make k6 work and want k6 archive and k6 cloud to "just work".

Partly because this ended up being the only not completely hackish way to know that we are past the initialization state which was needed in order to make a warning only in the init context.

See #3681

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 requested a review from a team as a code owner April 8, 2024 15:19
@mstoykov mstoykov requested review from oleiade and codebien and removed request for a team April 8, 2024 15:19
@mstoykov mstoykov added this to the v0.51.0 milestone Apr 8, 2024
@mstoykov mstoykov added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Apr 8, 2024
Copy link
Collaborator

@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.

The logic LGTM, just a request for improving the comment.

js/modules/resolution.go Outdated Show resolved Hide resolved
Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
@mstoykov mstoykov requested a review from codebien April 19, 2024 15:00
codebien
codebien previously approved these changes Apr 19, 2024
js/modules/resolution.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 73.33%. Comparing base (a62140c) to head (ba1af07).
Report is 7 commits behind head on master.

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

Files Patch % Lines
js/modules/resolution.go 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3680   +/-   ##
=======================================
  Coverage   73.33%   73.33%           
=======================================
  Files         278      278           
  Lines       20329    20338    +9     
=======================================
+ Hits        14908    14915    +7     
- Misses       4477     4479    +2     
  Partials      944      944           
Flag Coverage Δ
macos ?
ubuntu 73.28% <81.81%> (+0.01%) ⬆️
windows 73.18% <81.81%> (+0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mstoykov mstoykov merged commit edd2d15 into master Apr 22, 2024
22 checks passed
@mstoykov mstoykov deleted the lockModules branch April 22, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants