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

Local templates support in k6 new #4618

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

andrewslotin
Copy link
Contributor

@andrewslotin andrewslotin commented Mar 10, 2025

What?

This PR adds support for local templates and addresses a bug when an empty script file is created even if the template name is invalid.

Why?

The current script templating feature is limited to three pre-defined templates that are bundled with k6: minimal, protocol and browser. While this already reduces the amount of boilerplate code to write when starting a new project, being able to use custom user-defined templates would help to simplify the first steps even further, by allowing users to extract common parts, such as authentication, custom sets of options, into a template file that can be used to initialize tests.

Checklist

  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests for my changes.
  • I have run linter and tests locally (make check) and all pass.

Checklist: Documentation (only for k6 maintainers and if relevant)

Please do not merge this PR until the following items are filled out.

Related PR(s)/Issue(s)

Relevant issue #4154

@andrewslotin andrewslotin added feature documentation-needed A PR which will need a separate PR for documentation labels Mar 10, 2025
@andrewslotin andrewslotin added this to the v1.0.0-rc1 milestone Mar 10, 2025
@andrewslotin andrewslotin requested a review from dgzlopes March 10, 2025 14:48
@andrewslotin andrewslotin requested a review from a team as a code owner March 10, 2025 14:48
@andrewslotin andrewslotin requested review from inancgumus and oleiade and removed request for a team March 10, 2025 14:48
@andrewslotin andrewslotin force-pushed the local_templates_support branch from eda5a34 to b3bf79c Compare March 10, 2025 14:58
inancgumus
inancgumus previously approved these changes Mar 10, 2025
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Nice feature! LGTM 👍

I have some suggestions.

The --template option specifies the template type for the output. The current implementation of the --template option also reads the file name. Could this be confusing for users? What do you think about adding a --template-file option to explicitly specify the file name? Doing so can also simplify the implementation of this 🤔

$ ... --template=file --template-file=templateFile
# Or, just:
$ ... --template-file=templateFile

Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

Happy to see this feature land 🚀 👏🏻 Great job getting it through! 🙇🏻

No specific comments on the implementation, but I do share some of @inanc's sentiment on the UX. Passing a file directly to the option feels somewhat out of place, as, like he mentioned the existing values for the option are essentially "modes".

A pattern we currently use in k6 is to parse the value as a = separated key value pair: k6 new --template file=template.js, maybe that would make sense in this case? Otherwise, we could indeed go for --template-file, but the concern I have there is that we might want to support more than files down the road (folders?) in which case the "file" terminology might turn out confusing.

Two other small things I've noticed while playing around with it:

  1. In its current state, it looks like the ability to pass a template file is undocumented on the flag description:
    k6 new template
  2. The error message when providing a path to a file that does not exist looks somewhat incorrect "invalid template type", whereas it's really that the file was not found. Further, it could be more explicit? Something along the lines of "it appears you are trying to initialize a k6 project using a template file, but the template file you provided was not found".
    CleanShot 2025-03-11 at 10 36 19

@inancgumus
Copy link
Member

inancgumus commented Mar 11, 2025

@oleiade

A pattern we currently use in k6 is to parse the value as a = separated key value pair: k6 new --template file=template.js.

This makes sense to be consistent with the rest of the k6 command usage 👍 For example, log-output looks like this, which adds a file= case instead of parsing it (if it's a file) from a file prefix:

flags.StringVar(&gs.Flags.LogOutput, "log-output", gs.Flags.LogOutput,
	"change the output for k6 logs, possible values are 
	stderr,stdout,none,loki[=host:port],file[=./path.fileformat]")

Otherwise, we could indeed go for --template-file, but the concern I have there is that we might want to support more than files down the road (folders?) in which case the "file" terminology might turn out confusing.

We could call it --template-source. However, consistency matters more.

@andrewslotin andrewslotin force-pushed the local_templates_support branch from ce7349b to 1d2338d Compare March 11, 2025 16:10
@andrewslotin
Copy link
Contributor Author

@oleiade, good catch on unclear error and missing changes to the help message. Both should be fixed now.

@inancgumus, introducing a separate command-line flag for template files would allow users to do something like this --template minimal --template-file ./custom-template.js, which is an undefined behavior. To generate a script k6 new needs exactly one template, and using one flag to handle the value enforces this. I see your point that this may lead to a confusion, if someone tries to store and use template with exactly the same file name, as one of the built-in ones, but this seems an unlikely case and we can address it with user-friendly errors and documentation. For the context, I'm following this proposal and plan to add support for remote templates (at least ones that can be downloaded via HTTP/HTTPS). I'd rather avoid introducing yet another flag for URLs.

@inancgumus
Copy link
Member

inancgumus commented Mar 11, 2025

Thanks for the clarification, @andrewslotin 🙇 I now see that it was discussed and decided before. I wasn't aware of the conversation. However, this suggestion avoids introducing a new flag and remains consistent with the rest of the k6 command: k6 new --template=file=foo. I'm also fine with the current change if we want to go this route 👍 No more comments from my side :)

@andrewslotin
Copy link
Contributor Author

andrewslotin commented Mar 11, 2025

@inancgumus, what's the benefit of requiring users to type file= instead of providing a path if we can reliably infer what they meant? From UX perspective the latter is more convenient, because it works with TAB autocompletion and drag-and-drop of files supported by modern terminals. The key=value syntax is commonly used to provide key-value maps as arguments, like we do in --env. We require a single scalar value, while key=value format allows things like --template builtin=minimal,file=./custom-template.js.

@inancgumus
Copy link
Member

@andrewslotin Totally get your point—tab completion and drag-and-drop are super convenient. Nice points. My main concern is that string-based checks can be fragile; for instance, ~/foo doesn't work right now, and there might be more edge cases like that. And the error messages might feel a bit odd. If we had an explicit option like file=, we could rely on straightforward file-existence checks and avoid guessing. It also stays consistent with the k6's other flags, like log. But if we can handle those edge cases reliably, 💯 for prioritizing convenience and future extensibility!

@andrewslotin
Copy link
Contributor Author

@inancgumus, that would simplify the implementation for sure. Can you write down the full proposal you have in mind? Let's say we aim to support a small number of built-in templates, local files and remote ones. How would the arguments to --template look like in all these cases?

@inancgumus
Copy link
Member

inancgumus commented Mar 11, 2025

Given the constraints we discussed:

  • Avoid confusing users.
  • Keep a single --template flag to emphasize "one template" usage.
  • Don't use key-value syntax for external templates (e.g., file=...).
  • Prevent undefined behavior like --template minimal --template-file=foo.
  • Provide clear error messages and robust functionality.
  • Maintain easy drag-and-drop and tab completion.
  • Support built-in templates, files, directories, and remote protocols (e.g., HTTP).

So, I propose the following approaches.


Approach 1

# Built-in templates
k6 new --template builtin:minimal  # Explicit directive
k6 new --template builtin:protocol
k6 new --template builtin:browser

# HTTP etc.
k6 new --template https://foo.com

# Files and directories
k6 new --template ~/foo

How it works:

  1. Check if the argument starts with builtin: (builtin:foo errors out).
  2. If not, see if it's a valid remote path (http://, https://), which is straightforward.
  3. Otherwise, verify that it's a local file directly with fsext.ReadFile rather than using a heuristic string match.

Downsides

  • Users might type something like foo.com/foo and expect it to work as a remote URL.
  • However, given the single-flag constraint, this approach avoids extra flags and still offers clear semantics.

Using the builtin: prefix removes ambiguity with filenames. If someone just typed minimal, the tool would have to guess whether it was a built-in or a local file. By requiring builtin:minimal, we make it explicit.


Approach 2: if multiple flags were allowed

# Built-in templates
k6 new --builtin-template minimal   # Explicit directive
k6 new --builtin-template protocol
k6 new --builtin-template browser

# HTTP etc.
k6 new --remote-template foo.com    # Explicit directive

# Files and directories
k6 new --local-template ~/foo       # Explicit directive

For unexpected cases, we can clearly print out errors because everything is explicit:

k6 new --remote-template foo.com --local-template ~/foo
ERROR: Ambiguous template target. Use one of ... at the same time.

This would eliminate ambiguity, but it conflicts with the "one template flag" constraint.


Approach 3: A slightly less explicit and minimal approach

k6 new --template minimal
k6 new --external-template http://foo.com
k6 new --external-template ~/foo

It's still being arguably more explicit than a single template approach. However, unlike the previous one, this might not easily (or reliably) handle the foo.com case.

@andrewslotin andrewslotin force-pushed the local_templates_support branch from 3fd4607 to ab7f175 Compare March 12, 2025 10:29
@andrewslotin
Copy link
Contributor Author

andrewslotin commented Mar 12, 2025

@inancgumus, I believe we're reconsidering the previously discussed and agreed design now. Instead, I'd suggest that we focus on the implementation.

I see your point that requiring users to prepend the file name with ./ if a template is stored in the same folder as a new project might cause confusion; however, I believe this scenario is rather unlikely to happen. If someone has already copied the template file, there is not much benefit in running the k6 new command. However, to address this use case, a better error message would make sense – thanks for finding this.

The point about edge cases like using ~/, etc. is a good one. Although I've tried this locally and the ~ seems to be expanded to my home directory before passing this string to k6, I'm not 100% confident that it works the same way in other terminals/shells, so I've simplified the isFilePath() check to only look for a path separator in the string. This puts limitations on the built-in template names, as we won't be able to add one with a / or \\, but I believe it's a fair tradeoff. If we ever need to use such template name, we can iterate on the file path check and make it more sophisticated, but, again, we control built-ins, so I don't think this will cause us trouble anytime soon.

@andrewslotin andrewslotin requested a review from oleiade March 12, 2025 10:30
@andrewslotin andrewslotin force-pushed the local_templates_support branch from ab7f175 to 6ec040e Compare March 12, 2025 10:36
oleiade
oleiade previously approved these changes Mar 12, 2025
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

🚀 👏🏻 🍾 Looks good to me!

oleiade
oleiade previously approved these changes Mar 12, 2025
inancgumus
inancgumus previously approved these changes Mar 12, 2025
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

🤝 🚀

@andrewslotin andrewslotin dismissed stale reviews from oleiade and inancgumus via 6f99b50 March 12, 2025 17:19
@andrewslotin andrewslotin force-pushed the local_templates_support branch from 4ef4be4 to 6f99b50 Compare March 12, 2025 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-needed A PR which will need a separate PR for documentation feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants