Add a --dry-run flag to validate the config - #331
Conversation
F1bonacc1
left a comment
F1bonacc1
left a comment
There was a problem hiding this comment.
Hey @lopter,
Thanks for the PR! I think this is a very useful feature to have, and your approach is valid.
I think there is a simpler and more user-friendly way to tackle this issue.
If you add a --dry-run flag to the up and root commands, it will:
- Consider additional (existing and future) flags when validating.
- Eliminate the need for some of the refactoring.
What do you think?
b85f4ea to
3fec4c3
Compare
|
Thanks for the quick feedback @F1bonacc1, no strong feelings there, and I reworked the PR to go with your approach. I was wondering if Is there anything we can do regarding validation of the values? The config file uses a fair amount of enumerations that would be nice to validate as well. |
F1bonacc1
left a comment
F1bonacc1
left a comment
There was a problem hiding this comment.
Thanks, @lopter, for the quick refactor.
Regarding values validation, I see 2 main approaches here:
- Maintain
map[string]struct{}maps for all the valid values for each Key. This is a quick and simple approach, but can easily go out of sync. - Add code generation that will be generating both the
const stringsand the validation maps from someyamlorjsonschema files. This sounds to me like not an easy task. But I had a short back and forth with Gemini about it, and it gave me a decent head start.
What do you think? Want to give it a try?
| @@ -0,0 +1,11 @@ | |||
| { | |||
There was a problem hiding this comment.
Is this part of the PR or an accidental inclusion?
There was a problem hiding this comment.
Not accidental, but probably belong into another PR: if you don't mind I would like to add this file and wire it up from flake.nix. With a shell.nix, and maybe an .envrc, when a Nix user cd into the project, it sources everything needed to work on the project (that is even if they don't have golang installed on their machine).
| runner := getProjectRunner(args, *pcFlags.NoDependencies, "", []string{}) | ||
| if *pcFlags.DryRun { | ||
| processNames, _ := runner.GetLexicographicProcessNames() | ||
| fmt.Printf("Loaded %d processes from %d config files.\n", len(processNames), len(opts.FileNames)+len(opts.EnvFileNames)) |
There was a problem hiding this comment.
Do you think that "Loaded" really reflects the intent of this feature?
Maybe something like "Successfully validated" will make more sense for the --dry-run flag?
Yes, I think this will be the right approach. |
|
Great, I can set Regarding validating values, I see your two suggestions, and I am wondering if a third option could be to implement enums. This way, values would be validated when the config files are unmarshalled? |
|
I see both |
34967ea to
6c74c74
Compare
|
I did a quick test on one of the enums so that you can see what it would like, see: 6c74c74. |
Yes, I think everything can be |
I like your suggestion even more. |
|
qq: is there actually other user-facing enums than With those two enums in place, is there anything else we can do to validate the config? |
There is also the log level, but it is validated here: process-compose/src/loader/validators.go Line 26 in b0ca1d8 Other than that, I can't think of anything else. |
This allows Nix users to run `nix-shell` from the project's directory and bring in scope everything needed to use the Makefile. And use `/usr/bin/env bash` instead of `/bin/bash` for portability on Nix, and other OSes, where `bash` will not be installed in `/bin`.
8d69d08 to
9a2414f
Compare
|
Ok, I see it has I did some rebase/fixup/squash operation, looks like I introduced some issue and one of the test gets stuck. |
9a2414f to
4cff3c6
Compare
|
Test is fixed: I had to move |
lopter
changed the title
--dry-run flag to validate the config
|
I updated the PR title, and had one last question: Should we also allow dry-run to be set through an environment variable? |
I don't think so, at least not until someone will ask for it 😃 |
Without starting any processes. This is particularly useful in CI/CD scenarios. This is implemented using enums. `is_strict` is forced to true when `--dry-run` is used. The flag is set to `LoaderOptions` in order to do that.
86da121 to
98ac7d0
Compare
|
|
Alright, I don't have anything else then, this is ready to go for me :3 |
2 participants
Hello,
Thanks for the project, it is very useful.
One thing I have been missing is the ability to validate a generated config without starting any processes. This is very useful in CI, or with Nixpkgs when the config is generated as a derivation (and can be validated in the check phase of the derivation).
This PR attempts to add that, and I run into a couple questions I left in the code. Can you help answer them, and tell me if the overall approach is good?