fix: validate deploying module rather than entire schema #1217
Conversation
worstell
force-pushed
the
worstell/20240409-fix-module-validation
branch
from
7708773 to
2ece28b
Compare
backend/schema/validate.go
Outdated
| // Validate clones, normalises and semantically valies a schema. | ||
| func Validate(schema *Schema) (*Schema, error) { | ||
| // ValidateWholeSchema clones, normalises and semantically validates a schema. | ||
| func ValidateWholeSchema(schema *Schema) (*Schema, error) { |
There was a problem hiding this comment.
ubernit: the "Whole" here seems redundant, but also isn't consistent with ValidateModule() - schema.Validate() is more idiomatic because it doesn't "stutter", but schema.ValidateSchema() could be preferable now because we now have a few different Validate*() functions
backend/schema/validate.go
Outdated
|
|
||
| // ValidateModuleInSchema clones and normalises a schema and semantically validates a single module within it. | ||
| // If no module is provided, all modules in the schema are validated. | ||
| func ValidateModuleInSchema(schema *Schema, m *Module) (*Schema, error) { |
There was a problem hiding this comment.
I'm not 100% consistent with this myself, but I do try to use Optional in places where it conveys semantic meaning like it does here.
|
I wonder if we should "un"deploy any broken modules so that the cluster isn't in an inconsistent state. This will be unnecessary in the future because we'll allow multiple versions to be active at once, and broken state will also be prevented on a "real" full deploy anyway, so probably not necessary |
backend/schema/validate.go
Outdated
| @@ -60,7 +66,7 @@ func Validate(schema *Schema) (*Schema, error) { | |||
|
|
|||
| scopes := NewScopes() | |||
|
|
|||
| // Validate dependencies | |||
| // ValidateWholeSchema dependencies | |||
There was a problem hiding this comment.
Should the change to this comment and the one on line 84 be undone? It looks like "Validate" was used as a regular English language verb in these two places, rather than being a reference to the Validate function
There was a problem hiding this comment.
oop yes accidentally picked up from refactoring in my IDE! thanks for catching :)
with this change, when deploying a module, we will only validate the deploying module's schema (but do so within the context of the greater schema). this change prevents issues where a schema error outside of the deploying module prevents the deploy from succeeding—causing issues like #1211. now in combination with the buildengine logic, a change in the upstream module that breaks a downstream module will not prevent the change from deploying (if the deploying module still has a valid standalone schema). the deploy will succeed and then the subsequent deploy triggered for the downstream module will fail if the schema change from its dependency is breaking. fixes #1211
worstell
force-pushed
the
worstell/20240409-fix-module-validation
branch
from
2ece28b to
14f7140
Compare
when deploying a module, we will only validate the deploying module's schema (but do so within the context of the greater schema).
this change prevents issues where a schema error outside of the deploying module prevents the deploy from succeeding—causing issues like #1211. now in combination with the buildengine logic, a change in the upstream module that breaks a downstream module will not prevent the upstream module from deploying (if the module still has a valid standalone schema). the deploy will succeed and then the subsequent deploy triggered for the downstream module will fail if the schema change from its dependency breaks it.
fixes #1211