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

Clarify --include-retries docs and fixes #1341

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

plaindocs
Copy link
Contributor

  • switch options in examples to match usage
  • clarify --include-retries and add example

CC: @jayco

ticky
ticky previously approved these changes Nov 27, 2020
Copy link
Contributor

@ticky ticky left a comment

Choose a reason for hiding this comment

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

Liking this change! Got a couple of little bits of feedback on the words (which are mostly my own from the other day 😇)

clicommand/artifact_download.go Outdated Show resolved Hide resolved
Comment on lines +37 to +38
By default, only artifacts from the most recent job in a chain of retried jobs are downloaded.
To include artifacts from previous retried jobs, use the "--include-retried-jobs" flag:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this makes it clearer? “Retried” really feels like it does double duty here:

Suggested change
By default, only artifacts from the most recent job in a chain of retried jobs are downloaded.
To include artifacts from previous retried jobs, use the "--include-retried-jobs" flag:
By default, only artifacts from the last job in a chain of retried jobs are downloaded.
To include artifacts from superseded jobs, use the "--include-retried-jobs" flag:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm less keen on superseded, because if I read that I'd be wondering if it meant something specific in this context. We could just use "from previous jobs"

Huh, that was unintentional:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m just not sure “previous jobs” conveys “previous tries of this step” though, it seems ambiguous whether this means “prior jobs in the build” or something else

You can also use the step's job id (provided by the environment variable $BUILDKITE_JOB_ID)

By default, only artifacts from the most recent job in a chain of retried jobs are downloaded.
To include artifacts from previous retried jobs, use the "--include-retried-jobs" flag:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree. How about

Suggested change
To include artifacts from previous retried jobs, use the "--include-retried-jobs" flag:
To include artifacts also from all retried jobs belonging to this step, use the "--include-retried-jobs" flag:

"Also", to make clear that it is as well as the last one. What do you reckon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CC @ticky

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not sure this solves for the ambiguity in “retried,” and do we otherwise expose the step/job dichotomy to users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll ponder and reword tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We expose step just a couple of lines above, on line 35. But if we're not specifying a job ID, we're just using the artifact path, right, so this flag is just saying "get all the stuff, not just the most recent stuff", so mentioning the step isn't helpful. This might be better:

Suggested change
To include artifacts from previous retried jobs, use the "--include-retried-jobs" flag:
To include matching artifacts also from previous retried jobs as well as the most recent, use the "--include-retried-jobs" flag:

Base automatically changed from master to main February 1, 2021 05:25
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.

2 participants