-
Notifications
You must be signed in to change notification settings - Fork 104
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
[monorepo] add CI stage document for tests #4154
Conversation
/// Check run update recorded successfully in the respective CI stage. | ||
ok, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe add:
/// It is OK to evaluate returned results for stage completeness
/// The check run was found in the specified CI stage but the update failed. | ||
failed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Higher up:
log.info('$logCrumb: staging document not found for $transaction');
result: StagingConclusionResult.failed,
So the check run wasn't found either; but there was an internal error. Do we want that visibility here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the document wasn't found, how can we know whether the check run would be found in the doc or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming in a multi-staged world all documents are created and available? If so, lets note that somewhere so its clear "this is a failure because we expect the document to be there". Today that's not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either that or we have another conclusion. The comment is "the check run was found" - it wasn't because we don't have the document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha. Changed it to internalError
and updated the docs.
StagingConclusion stagingConclusion, | ||
RepositorySlug slug, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need stagingConclusion if its "close successful"?
Still LGTM |
fusionEngineBuild
completes, create a firestore document for thefusionTests
CI stage tasks.This PR does not yet alter the MQ guard behavior. I'd like to debug the document behavior first.
Progress towards flutter/flutter#159898