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
build: attach name of pipeline step to summary #4396
Conversation
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 like this change!
Name: stepName, | ||
Index: ps.curPipelineIndex() + 1, | ||
} | ||
line := logger.Blue(l).Sprintf("STEP %d/%d", ps.curPipelineIndex(), ps.totalPipelineStepCount) |
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.
Would this be simpler?
- remove the
PipelineStep.Index
field - remove
curPiplineIndex
and replacing withlen(ps.pipelineSteps)
(or with+ 1
or whatever) - remove
curPiplineStep
, and the new step immediately, and usepipelineSteps[len(pipelineSteps)-1]
in EndPipelineStep (which I think is the only place this gets used?)
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.
yyyup good call
internal/build/pipeline_state.go
Outdated
Index: ps.curPipelineIndex() + 1, | ||
} | ||
line := logger.Blue(l).Sprintf("STEP %d/%d", ps.curPipelineIndex(), ps.totalPipelineStepCount) | ||
l.Infof("%s — %s", line, stepName) | ||
ps.curBuildStep = 1 | ||
ps.curPipelineStepStart = ps.c.Now() |
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.
wanna pull this field into the PipelineStep
struct?
internal/build/pipeline_state.go
Outdated
@@ -35,7 +46,7 @@ const buildStepOutputPrefix = " " | |||
|
|||
func NewPipelineState(ctx context.Context, totalStepCount int, c Clock) *PipelineState { | |||
return &PipelineState{ | |||
curPipelineStep: 1, | |||
curPipelineStep: PipelineStep{Index: 0}, |
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.
IIUC it's idiomatic go to omit initializations to 0-values like this (both the Index
and the PipelineStep
here are such initializations)
(this is irrelevant if you agree with my below comment to delete curPipelineStep
)
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.
ha whoops yes. it used to be initialized to 1 and then i changed it but heh.
web/src/subdir/index.test.js
Outdated
@@ -0,0 +1,3 @@ | |||
test("everything is okay", () => { |
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.
👀
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.
This reverts commit 1672831.
@theothertomelliott asked what the steps a la--
in the web UI corresponded to.
these are steps in our update pipeline (e.g. "building dockerfile [gcr.io/xyz]", "Deploying to k8s"), which ARE given names at some point, but you need to scroll back up through the logs to associate the step and its name with the duration given below
this PR attaches the step name to its duration when we print the pipeline summary
I don't feel great about how the way we store
pipelineStep
andbuildStep
have diverged, but I figure this is fine for nowcc @hyu for "does this look ugly" consult