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

buildkite: Add tool to download logs #2777

Merged
merged 2 commits into from
Jun 25, 2019

Conversation

lukedirtwalker
Copy link
Collaborator

@lukedirtwalker lukedirtwalker commented Jun 19, 2019

This change is Reviewable

@lukedirtwalker lukedirtwalker requested a review from scrye June 19, 2019 13:41
Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker)

a discussion (no related file):
Downloaded archive names are difficult to read due to spaces in the name:

Unit tests :bazel:_etc.tar.gz

Replacing spaces with _ would improve readability.



go/tools/buildkite_log_downloader/main.go, line 33 at r1 (raw file):

var (
	tokenFlag = flag.String("token", "", "The buildkite API token")
	destDir   = flag.String("dst", "",

Feature request: I'd love it if the directory were created if it didn't exist already.


go/tools/buildkite_log_downloader/main.go, line 82 at r1 (raw file):

	}
	if len(problems) > 0 {
		return buildDesc{}, common.NewBasicError("Not all required flags provided", nil,

This error needs a bit of formatting to be easier to read. Right now I'm getting:

Not all required flags provided problems="[API-Token not provided Build number not provided]"%

The last character is probably due to a missing ending newline that my zsh is being nitpicky about.


go/tools/buildkite_log_downloader/main.go, line 108 at r1 (raw file):

	for _, artifact := range artifacts {
		if artifact.DownloadURL == nil {
			continue

Is it useful to inform the user of this event? E.g., "No URL found for artifact x".

It might point to some misconfiguration (I'm guessing, not that familiar with the buildkite api), and might be more useful than silently continuing.


go/tools/buildkite_log_downloader/main.go, line 111 at r1 (raw file):

		}
		if artifact.Filename == nil || !strings.HasPrefix(*artifact.Filename, "buildkite") {
			continue

Same remark as above.


go/tools/buildkite_log_downloader/main.go, line 115 at r1 (raw file):

		job := jobForArtifact(b, &artifact)
		if job == nil {
			continue

Same remark as above.

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @scrye)

a discussion (no related file):

Previously, scrye (Sergiu Costea) wrote…

Downloaded archive names are difficult to read due to spaces in the name:

Unit tests :bazel:_etc.tar.gz

Replacing spaces with _ would improve readability.

Done.



go/tools/buildkite_log_downloader/main.go, line 33 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Feature request: I'd love it if the directory were created if it didn't exist already.

Done.


go/tools/buildkite_log_downloader/main.go, line 82 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

This error needs a bit of formatting to be easier to read. Right now I'm getting:

Not all required flags provided problems="[API-Token not provided Build number not provided]"%

The last character is probably due to a missing ending newline that my zsh is being nitpicky about.

Done.


go/tools/buildkite_log_downloader/main.go, line 108 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Is it useful to inform the user of this event? E.g., "No URL found for artifact x".

It might point to some misconfiguration (I'm guessing, not that familiar with the buildkite api), and might be more useful than silently continuing.

Done.


go/tools/buildkite_log_downloader/main.go, line 111 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Same remark as above.

Done.


go/tools/buildkite_log_downloader/main.go, line 115 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Same remark as above.

Done.

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lukedirtwalker lukedirtwalker merged commit 2cdd3e7 into scionproto:master Jun 25, 2019
@lukedirtwalker lukedirtwalker deleted the pubBuildkiteLog branch June 25, 2019 08:16
lukedirtwalker added a commit to lukedirtwalker/scion that referenced this pull request Jun 27, 2019
There were some issues building the base image, which were introduced by scionproto#2777 and now fixed in this PR.
lukedirtwalker added a commit to lukedirtwalker/scion that referenced this pull request Jun 27, 2019
There were some issues building the base image, which were introduced by scionproto#2777 and now fixed in this PR.
lukedirtwalker added a commit that referenced this pull request Jun 27, 2019
There were some issues building the base image, which were introduced by #2777 and now fixed in this PR.
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