0.13: utilize validation handler #3997
Conversation
|
EDIT: unrelated, please ignore. Original commentRan a quick `cd examples; for example in $(ls -1); do pushd $example; echo $example; gdev13 validate; popd; done;` to see if all of our examples pass validation after these changes. Most of them pass fine! 🎉There are a couple of validation failures:
However, all of these failures also happen outside of this branch, so they are not new / caused by this extra validation ✅ Additionally, the This leaves just the |
|
@Walther Like the pr description says, as is, this pr does not change the behavior of the garden validate command. |
|
sorry, my bad 🤦♂️ should have read the description more carefully. thanks for the clarification! |
|
Do you think this is safe to merge before the pre-release, @edvald? I'm asking because we're adding a validation step here, which might result in new errors in certain places. |
|
Yes. Better to get those errors than something more cryptic/unexpected further down. |
core/src/garden.ts
Outdated
| @@ -876,7 +876,7 @@ export class Garden { | |||
| const resolvedProviders = await this.resolveProviders(log) | |||
| const rawModuleConfigs = await this.getRawModuleConfigs() | |||
|
|
|||
| const graphLog = log.makeNewLogContext({ section: "graph" }).info(`Resolving actions and modules...`) | |||
| const graphLog = log.makeNewLogContext({ section: "graph" }).info(`Reading actions and modules...`) | |||
There was a problem hiding this comment.
It looks like resolver.resolveAll() is called after this log message. Should we change the wording here?
There was a problem hiding this comment.
The old message can easily be restored while rebasing this on top of the 0.13 (there are some conflicts).
There was a problem hiding this comment.
The modules are resolved, but not the actions as far as I understand.
There was a problem hiding this comment.
Ah, got it, thanks for the clarification :) Please rebase this and then we can merge 🚀
There was a problem hiding this comment.
The method's JSDoc says
Resolve the raw module and action configs and return a new instance of ConfigGraph.
So, should we just keep the unified terminology in all places? We use "resolving" for providers too.
There was a problem hiding this comment.
To me either word is fine, please rebase this PR on op of 0.13. Then we can merge it and start the pre-release process.
The failures were caused by #3997 as the container validation handler actually modifies the deployment's ingress hostname to the project default hostname if it's not specified on the deploy itself.
The failures were caused by #3997 as the container validation handler actually modifies the deployment's ingress hostname to the project default hostname if it's not specified on the deploy itself.
This will call call the validation handler in the action resolution process.
This doesn't fix the original issue that brought this to our attention as
garden validatedoesn't actually resolve actions.Is that desirable? Should
garden validatealso resolve all static fields perhaps?