azure-service-operator icon indicating copy to clipboard operation
azure-service-operator copied to clipboard

Improve sequence enforcement of pipeline stages

Open theunrepentantgeek opened this issue 3 years ago • 7 comments

We currently have both Pre-requisites and Post-requisites on our pipeline stages; these are used to enforce a partial ordering on the stages to ensure they run correctly.

The two kinds of dependency are not isomorphic, but I've been queried on three separate times (by @babbageclunk, @matthchr and @super-harsh) which indicates the concept is not clear. This may be due to poor naming, poor documentation, or poor design (listed in no particular order).

Regardless of the cause, unclear code needs to be improved so that it makes sense future maintainers without needing recourse to a personal chat.

Discussion

The naming of pre-requisites seems clear; most of the confusion seems to come from post-requisites with the most common assumption being that they're just pre-requisites specified in reverse.

Comments made in earlier discussions (with minor editing for clarity):

  • The intent of the pre/post-requisites is to make sure the stages run successfully

  • Pre-requisites are a strong requirement - this stage cannot (and will not) work unless this other stage has already run.

  • If pre-requisites say you must have already run A if you want to now run B, then testing B requires the earlier stage A to be present. No choice. But if post-requisites say that C must come after B, you can test B without C.

  • We sometimes need ordering to ensure that one stages output doesn't pollute another's inputs. At one point we were generating JSON serialization tests for the extension types that Harsh introduced - those types never need to be serialized so generating those tests was wrong. As a temporary fix, we used a pre-requisite to ensure the serialization tests stage was run first (this has now been remedied).

  • The stage InjectJsonSerializationTests creates round trip serialization tests for any object types that have properties. It's not correct to give InjectJsonSerializationTests a prerequisite on every earlier stage that creates new object types becauses it isn't concerned with where those object came from. However, but those earlier stages DO want their new object types to be tested, so they declare a post-requisite on InjectJsonSerializationTests to ensure this happens.

theunrepentantgeek avatar Mar 10 '22 22:03 theunrepentantgeek

We still think this is worth doing

matthchr avatar Jul 11 '22 23:07 matthchr

We still think this is worth doing

matthchr avatar Jan 31 '23 00:01 matthchr

Goal is to allow people to compose a new pipeline for a new situation with some assurity that the result will work.

theunrepentantgeek avatar Jun 05 '23 23:06 theunrepentantgeek

We'd still like to do this

matthchr avatar Oct 30 '23 22:10 matthchr

Still probably worth doing but not a high priority currently

matthchr avatar Apr 08 '24 23:04 matthchr