Added the validation of template while creating it

[parauliya]

Description

This PR will validate a template. If timeout field for an action is not provided or provided with 0 value, then that will be considered as infinite timeout for that particular action.

Why is this needed

This fix is required so that Provisioner should only depends on global timeout.

How Has This Been Tested?

Added UT for the CreateTemplate API.
Also tested through e2e test to ensure that any existing feature doesn't break.

How are existing users impacted? What migration steps/scripts do we need?

No Impact on existig user

Checklist:

I have:

• updated the documentation and/or roadmap (if required)
• added unit or e2e tests
• provided instructions on how to upgrade

[codecov]

Codecov Report

Merging #280 into master will decrease coverage by 0.24%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
- Coverage   22.90%   22.65%   -0.25%     
==========================================
  Files          15       15              
  Lines        1275     1271       -4     
==========================================
- Hits          292      288       -4     
  Misses        964      964              
  Partials       19       19              

Impacted Files	Coverage Δ
pkg/template_validator.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdbf00a...8e106cc. Read the comment docs.

[gianarb]

That's nice. A couple of questions:

1. Is it better to have a default vs erroring when it is not passed?
2. this is a bc break, people using this code but with a workflow that does not have a timeout needs to know :) 0011: how to communicate bc-break proposals#15

[gauravgahlot]

@parauliya I remember you mentioned about having a default value for timeout, but I missed the bc break that time. You were right then.

@gianarb IMO it would be good to have a default, but we still want to make them aware of the new default. Because it is possible that the new default introduces some unexpected behavior.

Now, the question is what is the ideal (kinda) default timeout?

[gianarb]

You are right @gauravgahlot . Does not matter the decision we take, it is a bc break. I pinged @thebsdbox to figure out what we want because I don't really have a preference. Probably a default timeout applies here, but I am not strong about it

[parauliya]

@gianarb @gauravgahlot IMO, Since this is a action level timeout, if we have a default value for each action then there are following issues which can occur:

• There is a possibility the default value is much more than execution time for few actions, in that case, global timeout will be difficult to handle since we will be calculating the total time with the default values which can be more than global timeout provided in the template. And in that case, template will be invalid.
• The reverse of above is also possible where the time taken by an action is much more than the default value and in that case, the action will timeout.

Due to the above points I think it is better to make this field mandatory and user should provide this as a part of template.
For bc break I think we should think for some different approach.

If we talk about workflow timeout which is global timeout, this can have a default value which will be calculated dynamically by adding the timeout for all the actions and few additional seconds for communication over the network.

[gauravgahlot]

@gianarb @gauravgahlot IMO, Since this is a action level timeout, if we have a default value for each action then there are following issues which can occur:

• There is a possibility the default value is much more than execution time for few actions, in that case, global timeout will be difficult to handle since we will be calculating the total time with the default values which can be more than global timeout provided in the template. And in that case, template will be invalid.
• The reverse of above is also possible where the time taken by an action is much more than the default value and in that case, the action will timeout.

Due to the above points I think it is better to make this field mandatory and user should provide this as a part of template.
For bc break I think we should think for some different approach.

If we talk about workflow timeout which is global timeout, this can have a default value which will be calculated dynamically by adding the timeout for all the actions and few additional seconds for communication over the network.

Like @gianarb I don't have a preference as well.
As much as I would like the user to decide the timeout for each action and be flagged with an error when they don't, I don't want the current users to be confused as their templates fail to parse.

The point is what's more important here is to look for an efficient answer for How do we convey a bc break (like this one) to our existing users?

[gianarb]

How do we convey a bc break (like this one) to our existing users?

I linked a proposal about that

[gianarb]

@mmlb told me that internally Packet does not have a concept of per action timeout, but only a global one. And now I have big questions about life.

I think I would like to hold this PR until I can clearly figure out if it is needed, or if we should just forget about per action timeout :)

[nathangoulding]

@mmlb is right that Packet only has a global timeout. Making per-action timeouts mandatory seems overly burdensome for the end user.

IMHO, we need a global timeout for the workflow, an optional default timeout that applies to all actions, and (if we're feeling like it's necessary) per-action timeouts that can override the default timeout. I don't think that per-action timeouts should be mandatory.

[parauliya]

@nathangoulding Now the question is what should be the default value of timeout for actions?

[gianarb]

I had a chat with @thebsdbox and @nathangoulding to build some context around this feature.

Packet does not currently have a per-action timeout and it does not really need it, not sure why Tinkerbell has this feature BUT let's implement it in this way.

No validation required, by default the pre-action timeout is set to 0 and it means forever, so no-timeout.
Can you implement the feature in this way @parauliya ?
Thanks

[parauliya]

means

Sure, I am on it.

[mmlb]

I'd like to see the commit split into at least 2 commits, but not enough to actually block the PR on it.

[parauliya]

@gianarb @mmlb @detiber @gauravgahlot, Now the code cove report seems good.
Please approve the same.