Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add when' overload to accept a stage and treat stage result as a when condition for the enclosing context #76

Merged
merged 5 commits into from
Sep 11, 2024

Conversation

chrissimon-au
Copy link
Contributor

@chrissimon-au chrissimon-au commented Sep 9, 2024

Feedback welcome as this is my first engagement with this codebase.

Closes #75

Some concerns I have:

CancellationToken.None

As we need to run the stage during the condition evaluation of IsActive we don't have a cancellation token, so these whenStages aren't running as part of the normal execution model.

Is this an issue for you? If so, we might need to update the IsActive signature to accept a cancellation token.

Logging Output

Most of the logging logic makes a lot of assumptions about when in the execution it's being called. I've added a new StageIndex called WhenStage which allows us to log without an index. The logging result looks like:

For a pipeline when the when stage fails:

Run PIPELINE . Total timeout: -1ms.

Run stages

── WHEN STAGE  started. timeout: -1ms. step timeout: -1ms. ───────────────────────────────────────────────────────────────────────────────────────────────────────

/step-0> started
Error: Exit code is not indicating as successful.
/step-0> finished. 0ms. will trigger canncellation.
── WHEN STAGE  finished. 0ms. ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────


── STAGE #0  is inactive ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Run stages finished


Run post stages
Run post stages finished


PIPELINE  is finished in 1 ms

For a pipeline where the whenStage passes:

Run PIPELINE . Total timeout: -1ms.

Run stages

── WHEN STAGE  started. timeout: -1ms. step timeout: -1ms. ───────────────────────────────────────────────────────────────────────────────────────────────────────

/step-0> started
/step-0> finished. 0ms. 

── WHEN STAGE  finished. 0ms. ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────


── STAGE #0  started. timeout: -1ms. step timeout: -1ms. ─────────────────────────────────────────────────────────────────────────────────────────────────────────

/step-0> started
/step-0> finished. 0ms. 

── STAGE #0  finished. 0ms. ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Run stages finished


Run post stages
Run post stages finished


PIPELINE  is finished in 1 ms

When the when' is used at the pipeline level, the logging occurs BEFORE the Run PIPELINE message:

── WHEN STAGE whenStageShouldPass started. timeout: -1ms. step timeout: -1ms. ────────────────────────────────────────────────────────────────────────────────────

whenStageShouldPass/step-0> started
whenStageShouldPass/step-0> finished. 0ms. 

── WHEN STAGE whenStageShouldPass finished. 0ms. ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Run PIPELINE . Total timeout: -1ms.

Run stages

── STAGE #0  started. timeout: -1ms. step timeout: -1ms. ─────────────────────────────────────────────────────────────────────────────────────────────────────────

/step-0> started
/step-0> finished. 0ms. 

── STAGE #0  finished. 0ms. ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Run stages finished


Run post stages
Run post stages finished


PIPELINE  is finished in 1 ms

When the condition fails, this is especially odd as it shows the logs, then indicates that the verification failed and shows the conditions that failed, but it doesn't have access to the results of when the whenStage was first run, so I've updated the logs to refer back to the WHEN STAGE logs - the ? is because other conditions will show with green ticks and red crosses, as their results are static:

image

Is this ok? If there are multiple when stage` calls it might be a bit confusing, but hopefully the user knows to name the condition.

@chrissimon-au chrissimon-au changed the title Add when' overload to accept a stage and treat stage result as a when condition for the enclosing context Add when' overload to accept a stage and treat stage result as a when condition for the enclosing context (#75) Sep 9, 2024
@chrissimon-au chrissimon-au changed the title Add when' overload to accept a stage and treat stage result as a when condition for the enclosing context (#75) Add when' overload to accept a stage and treat stage result as a when condition for the enclosing context Sep 9, 2024
Copy link
Contributor

@albertwoo albertwoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and quick PR. Thanks.

Added some comments, please consider to changed. Many thanks for your contribution.

Fun.Build/ConditionsBuilder.fs Outdated Show resolved Hide resolved
Fun.Build/ConditionsBuilder.fs Outdated Show resolved Hide resolved
Fun.Build/StageContextExtensions.fs Outdated Show resolved Hide resolved
Fun.Build/StageContextExtensions.fs Outdated Show resolved Hide resolved
Fun.Build/StageContextExtensions.fs Outdated Show resolved Hide resolved
Fun.Build/StageContextExtensions.fs Outdated Show resolved Hide resolved
@chrissimon-au
Copy link
Contributor Author

Nice and quick PR. Thanks.

Added some comments, please consider to changed. Many thanks for your contribution.

Hi @albertwoo , please see updated PR which includes changes as suggested.

For the test to assert on the parent context, I simulated the scenario by setting an env var and using the env var in the condition stage. I verified that the test fails without setting the parent context, and passes when the parent context is set. I'm still not 100% clear on how the parent context structure works or is initialised, but hopefully this is correct.

@albertwoo albertwoo merged commit 541e4ba into slaveOftime:master Sep 11, 2024
1 check passed
@chrissimon-au chrissimon-au deleted the feat/whenStage branch September 11, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stage conditional on success of a run result
2 participants