Don't warn if parameter is only used as trigger #1618
Conversation
Parameters can be used to only trigger plans without being used in any templates. In this case no warning should be printed when verifying a package. Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
There was a problem hiding this comment.
this approach is incrementally better but leaves open the param.trigger to be nonsensical or assigned to a plan that was removed or has never been added yet.
It should be better to check that the Trigger
- is in the set of plans ✅
- if not in the set of plans, is warned that has plan assignment that is not in the set of plans
We should add a verify for checking that triggers are in the set of plans (covering the case of "used" params that are also triggers for plans that do not exist. I'll create an issue for that.
For this PR, I have slight preference that we check that the trigger is in the set of plans... If you choose to move forward with the current solution please create an issue for this enhancement.
| @@ -81,7 +81,10 @@ func paramsDefinedNotUsed(pf *packages.Files) verifier.Result { | |||
| } | |||
| for _, value := range pf.Params.Parameters { | |||
| if _, ok := tparams[value.Name]; !ok { | |||
| res.AddParamWarning(value.Name, "defined but not used.") | |||
| // A parameter could be use to trigger a plan while not being used in templates. | |||
| if value.Trigger == "" { | |||
There was a problem hiding this comment.
shouldn't we warn if the plan does not exist?
There was a problem hiding this comment.
We do that already, but in a different validation check that returns an error if a trigger is set to a non-existing plan. See https://github.com/kudobuilder/kudo/blob/main/pkg/kudoctl/packages/verifier/plan/verify_references.go#L37
|
My leaning has changed :), The more I think about it... there should be a verifier for triggers / plan names (which is performed irrespective of use). This should be captured as an error and would cover this situation. I'm not sure if that needs to be duplicated here. This seems 👌 the way it is but we need additional logic which I will capture in an issue. |
What this PR does / why we need it:
Parameters can be used to only trigger plans without being used in any templates. In this case no warning should be printed when verifying a package.
Fixes #1617