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

Hide "plan" comments #897

Merged
merged 12 commits into from
Apr 16, 2020
Merged

Hide "plan" comments #897

merged 12 commits into from
Apr 16, 2020

Conversation

goodspark
Copy link

@goodspark goodspark commented Jan 13, 2020

Addresses #721

Read commit by commit. Attempted to isolate changes.

Did not implement this for other VCS's. Github is my primary concern right now.

This will only hide "plan" comments, because IMO "apply" comments are important enough to have unminimized because they impact actual systems and thus may be useful when people need to audit changes or figure out what/how things broke.

I didn't test the entire flow, but did test my new code in isolation:

func TestSpark(t *testing.T) {
	c, err := NewGithubClient(
		"github.com",
		"goodspark-bot",
		"GETYOUROWNKEY",
	)
	if err != nil {
		t.Logf("client error: %v", err)
		t.FailNow()
	}
	r, err := models.NewRepo(
		models.Github,
		"goodspark/asd",
		"https://github.com/goodspark/asd.git",
		"",
		"",
	)
	if err != nil {
		t.Logf("repo error: %v", err)
		t.FailNow()
	}
	err = c.HideOldComments(r, 7)
	if err != nil {
		t.Logf("hide error: %v", err)
		t.Fail()
	}
}

See: goodspark/asd#7 for how it ran.

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

This is really cool, nice work!

Aside from my inline comments I'd like to see:

  • This turned on/off via a flag --hide-prev-plan-comments
  • pagination implemented

// a reasonable one, given we've already filtered the comments by the
// configured Atlantis user.
body := strings.Split(comment.GetBody(), "\n")
if !strings.Contains(body[0], models.ApplyCommand.String()) {
Copy link
Member

Choose a reason for hiding this comment

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

We should look for "Ran Plan for" and **Plan Error**.

Copy link
Author

Choose a reason for hiding this comment

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

I think we need to inject hidden comments in the body. If a plan output is too large, Atlantis seems to split them across comments without a common header.

Copy link
Member

Choose a reason for hiding this comment

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

Yes but it's super uncommon for that to happen in my experience. I think it's okay to not handle that edge case

Copy link
Author

Choose a reason for hiding this comment

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

How about we just search for "plan"?

server/events/vcs/client.go Outdated Show resolved Hide resolved
server/events/command_runner.go Outdated Show resolved Hide resolved
server/events/vcs/proxy.go Outdated Show resolved Hide resolved
@lkysow lkysow added the waiting-on-response Waiting for a response from the user label Jan 20, 2020
Sam Park and others added 9 commits January 24, 2020 21:51
The golang Github client recommends this.
And implement the Github version.

The Golang Github client recommends using shurcooL/githubv4.
So that every pull update will auto clean older comments
@goodspark goodspark requested a review from lkysow January 25, 2020 07:04
@codecov
Copy link

codecov bot commented Jan 25, 2020

Codecov Report

Merging #897 into master will decrease coverage by 0.05%.
The diff coverage is 68.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #897      +/-   ##
==========================================
- Coverage   71.74%   71.69%   -0.06%     
==========================================
  Files          65       65              
  Lines        5284     5348      +64     
==========================================
+ Hits         3791     3834      +43     
- Misses       1199     1216      +17     
- Partials      294      298       +4
Impacted Files Coverage Δ
cmd/server.go 79.26% <ø> (ø) ⬆️
server/user_config.go 100% <ø> (ø) ⬆️
server/events/command_runner.go 50% <0%> (-0.64%) ⬇️
server/events/vcs/not_configured_vcs_client.go 0% <0%> (ø) ⬆️
server/events/vcs/bitbucketcloud/client.go 46.61% <0%> (-0.72%) ⬇️
server/events/vcs/gitlab_client.go 38.65% <0%> (-0.67%) ⬇️
server/events/vcs/bitbucketserver/client.go 34.88% <0%> (-0.42%) ⬇️
server/events/vcs/azuredevops_client.go 68.81% <0%> (-0.75%) ⬇️
server/events/vcs/proxy.go 0% <0%> (ø) ⬆️
server/server.go 63.44% <100%> (+0.11%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0edf189...6a4236b. Read the comment docs.

@lkysow lkysow removed the waiting-on-response Waiting for a response from the user label Jan 25, 2020
@goodspark goodspark changed the title Hide comments Hide "plan" comments Feb 29, 2020
}

for _, comment := range allComments {
if comment.User != nil && comment.User.GetLogin() != g.user {
Copy link
Contributor

@mhumeSF mhumeSF Apr 14, 2020

Choose a reason for hiding this comment

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

Was testing this out but running into issues with case sensitive username.

Suggested change
if comment.User != nil && comment.User.GetLogin() != g.user {
if comment.User != nil && !strings.EqualFold(comment.User.GetLogin(), g.user)

Copy link
Member

Choose a reason for hiding this comment

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

Okay I'll add it.

@lkysow lkysow mentioned this pull request Apr 16, 2020
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

This is amazing work. I'm really sorry it took so long to review. I've updated it with master in #994 but kept all your commits of course. This will be in the next release 🎉

@lkysow lkysow merged commit 6a4236b into runatlantis:master Apr 16, 2020
@goodspark goodspark deleted the hide-comments branch April 16, 2020 18:06
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