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

cmd/github-post: refactor issue posting into a library #25596

Merged
merged 2 commits into from
May 17, 2018

Conversation

petermattis
Copy link
Collaborator

See #24894

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis petermattis requested review from benesch and tbg May 16, 2018 22:03
@petermattis
Copy link
Collaborator Author

Hmm, this is primarily code movement, but reviewable isn't showing it as such. Let me try to structure the commits show that the actual diff shows up better. I probably won't get to this until tomorrow.


Review status: 0 of 5 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@petermattis petermattis force-pushed the pmattis/github-post branch 2 times, most recently from ed4e76d to 8d927ef Compare May 16, 2018 22:15
@tbg
Copy link
Member

tbg commented May 17, 2018

I can take another look when you've rejiggered this, but as long as you've tested it end-to-end and there aren't any substantial changes in semantics here, LGTM.


Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


pkg/cmd/internal/issues/issues.go, line 58 at r1 (raw file):

const githubIssueBodyMaximumLength = 1<<16 - 1

func trimIssueRequestBody(message string, usedCharacters int) string {

Explain what message is with an example (I tried unsuccessfully to find out).


pkg/cmd/internal/issues/issues.go, line 62 at r1 (raw file):

	if m := stacktraceRE.FindStringIndex(message); m != nil {
		// We want the top stack traces plus a few lines before.

This doesn't have to be in this PR, but some of these messages can get quite long (see for example #25573).

It might be nice to use the

<details>
 <summary>Summary Goes Here</summary>
collapsed content
</details>

construct whenever a good summary is known. That would reduce the time spent scrolling vigorously while looking at test flakes.


Comments from Reviewable

No changes. This is a pure copy of the code to ease subsequent diffs.

Release note: None
@petermattis petermattis force-pushed the pmattis/github-post branch 2 times, most recently from 6162a59 to 1a8e4d1 Compare May 17, 2018 14:08
@petermattis
Copy link
Collaborator Author

There are no semantic changes here, just reorganization of code. github-post now only cares about parsing go test output and issues.Post is what creates a github issue or adds a comment to an existing issue. This logic was previously all bundled together in github-post.

The one tiny change is that the body of the issue message now says Failed test instead of Stress build found a failed test.


Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.


pkg/cmd/internal/issues/issues.go, line 58 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Explain what message is with an example (I tried unsuccessfully to find out).

Not my code. Comment added with my understanding.


pkg/cmd/internal/issues/issues.go, line 62 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

This doesn't have to be in this PR, but some of these messages can get quite long (see for example #25573).

It might be nice to use the

<details>
 <summary>Summary Goes Here</summary>
collapsed content
</details>

construct whenever a good summary is known. That would reduce the time spent scrolling vigorously while looking at test flakes.

Agreed this would be nice. Do you know how to achieve this? I've added a TODO.


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

bors r=tschottdorf

@craig
Copy link
Contributor

craig bot commented May 17, 2018

Canceled

@petermattis
Copy link
Collaborator Author

bors r=tschottdorf

craig bot pushed a commit that referenced this pull request May 17, 2018
25596: cmd/github-post: refactor issue posting into a library r=tschottdorf a=petermattis

See #24894

Release note: None

Co-authored-by: Peter Mattis <petermattis@gmail.com>
@craig
Copy link
Contributor

craig bot commented May 17, 2018

Build succeeded

@craig craig bot merged commit 50733d5 into cockroachdb:master May 17, 2018
@petermattis petermattis deleted the pmattis/github-post branch May 17, 2018 16:25
@tbg
Copy link
Member

tbg commented May 17, 2018

🎆 🍾 I owe you one @petermattis

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.

3 participants