Skip to content

Fix job unblock data precedence - #927

Draft
nobradov1c wants to merge 1 commit into
buildkite:mainfrom
nobradov1c:nobradovic/fix-unblock-data-precedence
Draft

Fix job unblock data precedence#927
nobradov1c wants to merge 1 commit into
buildkite:mainfrom
nobradov1c:nobradovic/fix-unblock-data-precedence

Conversation

@nobradov1c

Copy link
Copy Markdown

Summary

  • prefer an explicitly supplied --data value over stdin in bk job unblock
  • preserve piped stdin as the fallback when --data is absent
  • add regression coverage for empty non-TTY stdin (/dev/null)

Fixes #926.

Testing

  • go test ./cmd/job
  • go vet ./cmd/job
  • git diff --check

I also ran go test ./...; all relevant packages passed, but the existing keyring tests failed because this development environment has an active shared-memory credential store.

Comment thread cmd/job/unblock.go
@@ -89,14 +89,16 @@ func (c *UnblockCmd) Run(kongCtx *kong.Context, globals cli.GlobalFlags) error {
}

func (c *UnblockCmd) unblockFields() (map[string]any, error) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice work!
One small suggestion on the test though, right now it works by temporarily swapping out the program's input with a placeholder one, then switching it back after the test is done. That works, but I think it would be safer to just enter the test input directly into the function instead of swapping anything out. The reason is that if another test happens to run around the same time, they could end up interfering with each other.

Comment thread cmd/job/unblock_test.go
}
}

func TestUnblockFieldsPrefersDataOverEmptyNonTTYStdin(t *testing.T) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The other thing is, right now there's only one test, and it covers the case where both --data and manually entered data are given, confirming that --data correctly wins. But this change touches two other situations as well, when data is entered manually with no --data at all, and when neither one is provided, so it'd be worth adding tests for those too. That way we can be confident the reordering didn't affect anything that already worked before this fix.
None of this is blocking, just some points to improve on before it merges

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.

job unblock --data is ignored when stdin is a non-TTY with no data

2 participants