-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
feat: add a new .taskrc.yml to enable experiments #1982
base: main
Are you sure you want to change the base?
Conversation
Did you ever consider multidoc yaml files? Then you can have many schemas within a single Taskfile.yml, and solve other interesting problems related with advanced Task integrations (see #1916). Changes in and around reader.go, but nothing significant. ---
version: 3
tasks: ...
---
task-experiments:
TASK_X_FOO: BAR
---
foreign-schema:
ignored-by-task: ... The current mechanism for enabling experiments is a bit cumbersome. It would be ideal to have the option to also configure this directly in the Taskfile. Additional files is perhaps not helping so much, for instance with remote taskfiles where the remote taskfile would need one of the experiments enabled. |
Great review as always @pd93 thanks! Regarding the filename, how would you like to proceed? Should I post a message on Discord to discuss it? @trulede, about configuring in the Taskfile itself, @pd93 already answered here #1978 (comment) It can be tricky to manage if, for instance, three includes define the same experiments (e.g., ANY_MAPS) but with different values. |
I was suggesting using a multi-doc yaml file, containing the Taskfile (schema) and other schemas in subsequent documents contained within the same yaml file. It just something to consider as you can place multiple docs/schemas in a single YAML file. The question I might have in respect to this PR is why not use the existing mechanism There is a more fundamental problem here; if I design a Taskfile to use an experimental feature, then that feature may no longer optional, so I need a way to ensure that its enabled. Adding a new schema to the YAML file which contains the taskfile schema would be a handy way to achieve that. Adding a |
Although putting multiple schemas in a single YAML file is possible, I'm not keen on this approach. Experiments were not designed to be used in production (hence the name) and I have no intention to design large features just to support cases where users have experiments enabled across teams. I would prefer to keep experimental code far away from the schema as experiments are inherently very changeable. However, I do understand that people will and are using experiments across teams and that there needs to be some way (as @nathanperkins requested) to allow this.
Can you explain (other than file conflicts) why you think the Another reason that using the schema (including the dotenv) is not practical is because the Taskfile is not read/parsed and the dotenvs are not loaded until a significant amount of the code has executed. We may need/want to have experimental flags for things that run before a Taskfile is parsed. This is why the experiment flags are one of the first things to be evaluated in our code.
Maybe I'm misunderstanding, but as far as I can tell loading an extra file such as
Loading from |
I think we can just leave this issue open for a bit (no rush to merge) and let @andreynering and other members of the community leave some feedback on the change and filenames. Feel free to put a message on Discord that links people here, but not everyone has a Discord account, so GitHub is a more transparent place for this feedback. |
Of course, no need to rush at all 🙂 For the filename, I can think at :
Feel free to comment and suggest filename |
Hey guys, What problem are we solving by having a separate config file? Can't the user just commit the If we decide to proceed, I strongly suggest |
Also, a more generic name means we can expand to contain more settings in the future if needed, so having a specific name like |
The only problem I really have with
Not against this given the industry standard, though I always found this precedent weird since
The experiments specific file was really just to avoid backwards incompatible changes with schemas. For example, if we include settings in the same file (regardless of filename), we will occasionally be removing valid settings from the config file. Having said that, we could just set the schema to a |
From Claude (pasting just out of curiosity):
Yes, in theory we could just ignore any values. That said, I like the @nathanperkins idea (mentioned here and here) that we would error if an unrecognized / retired experiment is enabled, to avoid surprises to the user. |
From ChatGTP, it seems it also mean "runtime configuration" . We also have
I also put this idea on the table here #1978 (comment)
Yes, I started type as |
Ahah @andreynering I just did the same with ChatGPT 😂 |
@andreynering @vmaerten Interesting to see your AI tools of choice 😆 TIL it stands for runtime configuration too. I had only ever heard the runcom thing. In that case, this makes a lot more sense and I'm totally fine with it. Sounds like the consensus is I fine with erroring on unrecognised experiments. There is actually a leftover line that does this for the old |
Yes, at least for me!
I'm fine with it but it could / should be done in another PR |
So for example, to enable map variable second version, we could do : experiments:
MAP_VARIABLES: 2
|
Question: Do we want a schema version: 3
experiments:
MAP_VARIABLES: 2 My opinion is probably yes to both of these. Main reason being that it might be confusing to have different versions for different things. |
Good point. I'm not sure, because I don't expect this schema to change much, if at all. |
Haha, I'm gunna screenshot this comment and save it for a couple of years 😉 |
Haha, for now, we'll only have experiments in it. Maybe we can delay the decision until we add other things? |
I've updated the code based on what we discussed. Currently, the parsing is done in experiment, but we'll move it to a more global scope if we decide to use this file for other settings. I've also created a JSON Schema hosted directly in our website. For now, I did not add version but it can be challenged |
If you put an "experiment" into the Task executable, it's not an experiment, it's a feature. I understand the intention, but in doing so its created a config issue, and a difficult user experience. A plugin mechanism might be better, and open up Task to external developers. Does Task really need a config? IMO nothing is being achieved with the current mechanism. If I design a task which uses an "experimental" feature, then I want to use that feature. Having some kind of enabling mechanism is just complicating the use of Task. Nothing else. In any case, I suggest following the traditional patterns for config files (git does config this way). Start with something general (
You can take it a long way ... that is the problem with configs. |
Correct. |
I don't understand what the alternative to having experiments in the executable would be. Where else would they go? Experiments are experimental features. They are things that we want to get feedback on before they are added without us having to make backwards compatible changes.
I don't believe there is a config issue. The way it works today is fine and has been since it was introduced. We're simply introducing an alternative to
Plugins are complex to develop in a cross-platform way. Also, plugins tend to "plug in" to a specific part of a system. Experiments need to be able to modify any aspect of the code. Plugins are not a suitable solution here.
Experiments aren't always a case of "use it or don't". They are allowed to (and usually do) change behaviour. This means two users with the same task can see different behaviour if they have different experiment flags enabled. Its not complicating anything - It is necessary to stop us from breaking users projects.
Yes, if we have a proper settings config, then maybe one day it will be hierarchical like you suggested, but this is a long way out of scope for this issue/PR. We are not implementing settings here. We are implementing an alternative way of enabling experiments which may use the same file as a potential future settings file.
As discussed in this thread |
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've updated the code based on what we discussed. Currently, the parsing is done in experiment, but we'll move it to a more global scope if we decide to use this file for other settings.
Looks good! I left a couple of minor comments
I've also created a JSON Schema hosted directly in our website.
Once the PR is merged we should submit this to https://github.com/SchemaStore/schemastore
Reference to the main Taskfile schema (It just refers to the one we host on https://taskfile.dev) and the catalogue entry
For now, I did not add version but it can be challenged
I personally lean towards adding it now so that we don't have to maintain support for no version when we inevitably decide to add it later. However, lets get everyone aligned on this before you change anything.
} | ||
|
||
type ExperimentConfigFile struct { | ||
Experiments map[string]string `yaml:"experiments"` |
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.
Question: Currently the only acceptable values for experiments are integers (1, 2). However, technically the value is saved as a string (mostly because env vars are always strings). If we think that we will never use anything other than integers, then maybe the values in the config file should be ints:
Experiments map[string]string `yaml:"experiments"` | |
Experiments map[string]int `yaml:"experiments"` |
Open to ideas about why we'd want to support strings though.
".taskrc.yaml", | ||
} | ||
|
||
type ExperimentConfigFile struct { |
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.
No need to export this right now. Maybe in the future if we move it somewhere else.
Starting to think about what the package API should look like with the aim of stabilising it at some point, so I'm trying to limit what we export where possible.
@@ -39,23 +42,41 @@ Which method you use depends on how you intend to use the experiment: | |||
1. Prefixing your task commands with the relevant environment variable(s). For | |||
example, `TASK_X_{FEATURE}=1 task {my-task}`. This is intended for one-off | |||
invocations of Task to test out experimental features. | |||
1. Adding the relevant environment variable(s) in your "dotfiles" (e.g. | |||
2. Adding the relevant environment variable(s) in your "dotfiles" (e.g. |
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.
FYI. Technically no need to number these explicitly. Markdown will render an ordered list sequentially regardless of what numbers you use. This is supposed to make reordering easier, but doesn't really make a difference here.
The problem with It works, yes, but operationally the use of experiments is a nuisance. I understand the intention, but here is how it goes in a development cycle: 1/ Decide to use a remote task file. Actually, we use versioned Taskfiles (git tags) and associated containers, from remote repos - so this is a very powerful usage pattern. Point 6 is important. We accept the behaviour will change, or disappear. Now you get the feedback, I guess. Feature is great, operation is very un-task-like.
It's not a great choice. 'rc' historically stands for "run commands" which is semantically different to configuration. Also, are you sure about versioning and a schema for a config file? Take a look at how other config files work; git or docker; that is not normally how config files are implemented. Of course, nothing wrong with it either. It's much easier if switches for behaviour are represented in some kind of normal way. Environment variables, at the root of the taskfile schema, would be ideal. Simple, versioned schema already exists, can simply parse the taskfile early in the process and extract the necessary content. No practical difference to the current Only seems to be adding complexity to me. But as I explained above, I've avoided the config file, so I have no skin in the game. |
I like this idea !
This file should override the environment variable.