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 status reason messaging #833
Conversation
* Remove unnecessary fields from StepFinishedParams * Update status reasons * Introduce the new StepRunResultsModel.StatusReasons function * Version -> FormatVersion * Remove unused method * Handle switch cases exhaustively * Update unit tests * Update unit test Co-authored-by: vikas <vikas.shah@bitrise.io>
// TODO: if len(titleWithSuffix) > titleBoxWidth because of a long suffix, | ||
// might we trim too much from the title |
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.
Is this still a todo?
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.
Yes it is. The mentioned issue won't come up in real Bitrise builds, if we consider the current max box width (stepRunSummaryBoxWidthInChars
), the lenght of titles, and title statuses. But still in itself won't work as expected for arbitrary input.
isExitStatusError := true | ||
if err != nil { | ||
isExitStatusError = errorutil.IsExitStatusError(err) | ||
} | ||
|
||
// Print step preparation errors before the step header box, | ||
// other errors are printed within the step box. | ||
if status == models.StepRunStatusCodePreparationFailed && err != nil { | ||
if !isExitStatusError { | ||
log.Errorf("Preparing Step (%s) failed: %s", pointers.StringWithDefault(stepInfoCopy.Step.Title, "missing title"), err) | ||
} | ||
} |
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 prefer this way when every step related error appears inside the step box. Even if technically for these step preparation errors the step was not started. But does this new behaviour needs to be coordinated with the FE team? Because previously this error was appearing as a bitrise cli error before the step start event and now the error only appears in the step finished event.
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.
AFAIK bitrise CLI logs are not shown when structured logging is enabled, this mostly changes how the console log is presented.
cli/run_test.go
Outdated
return strings.Contains(output, expectedEvent) | ||
} | ||
assert.Eventuallyf(t, condition, 5*time.Second, 150*time.Millisecond, "", "") | ||
//assert.True(t, strings.Contains(output, expectedEvent), output) |
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.
Removable comment?
condition := func() bool { | ||
output := buf.String() | ||
return strings.Contains(output, expectedEvent) | ||
} | ||
assert.Eventuallyf(t, condition, 5*time.Second, 150*time.Millisecond, "", "") |
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.
Why is this new assert needed? From what I can see in the test that we save the loggers output into a buffer and run a test workflow. By the time we reach this assertion the workflow run should be over and we should have the output in the buffer. I think we do not do any concurrent operation in the cli (workflow related).
I was curious and restored the previous assertion and did 5 test runs locally and it was always passing. Is this flaky on the CI?
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.
The test case (GivenPluginRegisteredForMultipleTriggers_ThenPluginTriggeredTwice
) is flaky on CI, here is an example build: https://app.bitrise.io/build/0e707cfb-7591-4465-b6ab-e57f6e28925f.
if mergedStep.RunIf != nil { | ||
stepInfoPtr.Step.RunIf = pointers.NewStringPtr(*mergedStep.RunIf) | ||
} | ||
|
||
if mergedStep.Timeout != nil { | ||
stepInfoPtr.Step.Timeout = pointers.NewIntPtr(*mergedStep.Timeout) | ||
} | ||
|
||
if mergedStep.NoOutputTimeout != nil { | ||
stepInfoPtr.Step.NoOutputTimeout = pointers.NewIntPtr(*mergedStep.NoOutputTimeout) | ||
} |
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.
These are needed because they were not set on the stepInfoPtr
previously? I was trying to see where we are reading these from the stepInfoPtr
but could not see it. Is this a change to just make the code technically correct?
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.
These are needed because they were not set on the stepInfoPtr previously?
Yes, if you search for stepInfoPtr.Step.
in the activateAndRunSteps func, you can see that we only set the step properties that are later used for presentation (step boxes).
stepInfoPtr
is passed to the buildRunResultCollector.registerStepRunResults
, which uses it for creating the stepResults:
- https://github.com/bitrise-io/bitrise/blob/CI-318/cli/build_run_result_collector.go#L73
- https://github.com/bitrise-io/bitrise/blob/CI-318/cli/build_run_result_collector.go#LL92
Then the added fields are used for creating the status reason: https://github.com/bitrise-io/bitrise/blob/CI-318/cli/build_run_result_collector.go#L184
for _, stepError := range params.Errors { | ||
lines = append(lines, colorstring.Red(stepError.Message)) | ||
} | ||
if params.StatusReason != "" { | ||
lines = append(lines, colorstring.Blue(params.StatusReason)) | ||
} |
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 have to admit you tricked me with this clever solution. 😂
I set a step preparation failed error and was debugging who prints the error inside the step box because at first look it did not make sense. The step did not run and still someone printed the error there. And if I ran it with json output then there were only the step started and step finished events and nothing in between.
But then I pieced it together and realised that the step footer now prints the errors and status reason as part of the main step body. It is was tricky for me to understand because my mind thought that the step footer always starts with the step timing information.
I do not have a better solution but just wanted highlight that this might be a little bit confusing the first time you encounter it.
models/step_run_status.go
Outdated
const ( | ||
StepRunStatusCodeSuccess StepRunStatus = iota | ||
StepRunStatusCodeFailed | ||
StepRunStatusCodeFailedSkippable | ||
StepRunStatusCodeSkipped | ||
StepRunStatusCodeSkippedWithRunIf | ||
StepRunStatusCodePreparationFailed | ||
StepRunStatusAbortedWithCustomTimeout // step times out due to a custom timeout | ||
StepRunStatusAbortedWithNoOutputTimeout // step times out due to no output received (hang) | ||
) |
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 want to highlight that in the original constant list the value 6 was missing. So StepRunStatusCodePreparationFailed
was 5 and StepRunStatusAbortedWithCustomTimeout
7. With this change the last two statuses will not have their original value.
And this value reordering and using the iota might be dangerous ot use. If someone inserts a new state in the middle of the list (or reintroduces the missing 6 value) then the whole status is off. And this is used by (based on GitHub code search):
- build log service
- bitrise analytics plugin
and based on the code they use it for mapping purposes.
Either lets use dedicated integer values like before or add a big comment to warn not to modify the order of the list because other tools might depend on them.
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.
Thanks for spotting this, it was not intentional, restored here: b58b743
models/models.go
Outdated
return "" | ||
case StepRunStatusCodeFailed, StepRunStatusCodePreparationFailed, StepRunStatusCodeFailedSkippable: | ||
return fmt.Sprintf("exit code: %d", exitCode) | ||
func (s StepRunResultsModel) StatusReasons() (string, []StepError) { |
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.
The method returns more than what the name suggest. I do not have a great suggestion but I would rename it to StatusReasonAndErrors
or FinalStatus
or something else.
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.
We decided to use this name because both the status_reason and the errors are reasonings for the given status.
Checklist
README.md
is updated with the changes (if needed)Version
Requires a MINOR version update
Context
This change introduces better status reason messaging for steps for the build log grouping feature.
The relation between step run
status
,status_reasons
anderrors
are here:Changes
StatusReasons
method is now part of theStepRunResultsModel
struct, which returns the status_reason and/or errors related to the given step run status.3661 -> "1h 1m 1s"
Investigation details
Decisions