Skip to content

Conversation

@VikaCep
Copy link
Contributor

@VikaCep VikaCep commented Sep 1, 2025

Closes #1243

As we're implementing Version Management, check versions will be selected by choosing the corresponding channel from the k6 channel selector (see design doc and slack conversation)
This means setting the version restriction in the script is not encouraged, in fact it's not valid (at least for now) as the backend won't honor it. k6 extensions are also not allowed. For this reason I'm adding the respective UI validation both to the form schema and Monaco editor:

  • Error message when importing k6 extensions
image
  • Error message when using k6 pragmas
image

@VikaCep VikaCep self-assigned this Sep 1, 2025
@VikaCep VikaCep marked this pull request as ready for review September 1, 2025 17:56
@VikaCep VikaCep requested a review from a team as a code owner September 1, 2025 17:56
@VikaCep VikaCep requested a review from ckbedwell September 1, 2025 17:56
@github-actions
Copy link

github-actions bot commented Sep 1, 2025

Script size changes

Name +/- Main This PR Outcome
[module.js] +0.14% 2,386.30 kB 2,389.65 kB
[datasource/module.js] = 24.68 kB 24.68 kB

Totals

Name +/- Main This PR Outcome
[Scripts] +0.14% 2,410.98 kB 2,414.33 kB
[Non-script Assets] = 2,587.82 kB 2,587.82 kB
[All] +0.07% 4,998.80 kB 5,002.15 kB

Generated by 🚫 dangerJS against bcf519d

@VikaCep VikaCep force-pushed the feat/vm-reject-pragmas branch 2 times, most recently from 2ec0e80 to a08a28c Compare September 5, 2025 18:22
@VikaCep VikaCep requested a review from w1kman September 5, 2025 18:26
@VikaCep VikaCep force-pushed the feat/vm-reject-pragmas branch from a08a28c to 6f92b4c Compare September 18, 2025 15:00
ckbedwell
ckbedwell previously approved these changes Sep 19, 2025
Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

I've left a comment but it's quite small so I'm going to approve and leave it to you to decide if you want to address it or not 🙂

@VikaCep
Copy link
Contributor Author

VikaCep commented Sep 19, 2025

I've addressed your comments @ckbedwell

Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

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

I don't feel that this change actually checks for JavaScript directives, rather it uses a regular expression to find a match.

Image

monaco is way too advanced for not allowing to check specifically for directives, rather than matches, anywhere.

Also, I'd expect the example directives in k6deps to trigger the error.

Image

@w1kman
Copy link
Contributor

w1kman commented Sep 24, 2025

Seems possible. Did a simple validation rule for "use k6" (It does not apply to version req).

image

@w1kman
Copy link
Contributor

w1kman commented Sep 24, 2025

See how only the actual "directive" matches the "Thomas Wikidation" rule

"use k6 >= 1";
image

const myVar = "use k6 >= 1"
image

console.log("use k6 >= 1");
image

// "use k6 >= 1"
image

Note: It's possible to add "Quick fix" to remove directive.

@w1kman
Copy link
Contributor

w1kman commented Sep 24, 2025

Screen.Recording.2025-09-24.105312.mp4

@VikaCep VikaCep force-pushed the feat/vm-reject-pragmas branch from 962c560 to fa93bb4 Compare September 24, 2025 18:02
@VikaCep
Copy link
Contributor Author

VikaCep commented Sep 24, 2025

Thanks for the review @w1kman, I've refactored the implementation to use acorn AST parsing instead, which makes the validation more robust and context-aware. The new approach correctly distinguishes between actual k6 directives and things like comments or strings in console.log statements, while also adding Quick Fix actions to automatically remove forbidden code.
Let me know if this looks better to you :)

Screen.Recording.2025-09-24.at.17.01.43.mov

@VikaCep VikaCep requested a review from w1kman September 24, 2025 20:05
VikaCep and others added 4 commits October 7, 2025 17:58
* feat: add custom validation to monaco

* fix: improve regex and minor changes

* fix: tests

* fix: lint

---------

Co-authored-by: Virginia Cepeda <[email protected]>
@VikaCep VikaCep force-pushed the feat/vm-reject-pragmas branch from 3678373 to bcf519d Compare October 7, 2025 20:58
Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

LGTM! Great job! 🥇

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ these tests ❤️

Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

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

LFTM 🥳

@VikaCep VikaCep merged commit ae72443 into main Oct 14, 2025
23 checks passed
@VikaCep VikaCep deleted the feat/vm-reject-pragmas branch October 14, 2025 17:49
@sm-release-app sm-release-app bot mentioned this pull request Oct 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reject scripts that import extensions or include pragmas

3 participants