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

Update Ike's reaction in the PR if the state changes #145

Closed
gastaldi opened this issue May 7, 2018 · 6 comments
Closed

Update Ike's reaction in the PR if the state changes #145

gastaldi opened this issue May 7, 2018 · 6 comments

Comments

@gastaldi
Copy link
Contributor

gastaldi commented May 7, 2018

About the plug-in that checks for tests in a PR, it would be nice if Ike's reaction (in the PR comment) were updated when the PR is updated.

Even though that the PR status changes accordingly, updating the comment with Ike's feedback would be a nice visual indicator for PR reviewers ;)

@maxandersen
Copy link

isn't this more apropriately done by a new comment + update via status api ?

@gastaldi
Copy link
Contributor Author

gastaldi commented May 7, 2018

@maxandersen I'd prefer to update the existing comment instead of adding new comments as this can confuse someone when reviewing the PR (specially if it has a lot of comments)

@maxandersen
Copy link

then I would say better to use status api updates rather than comments. ie. like a failed build or linting check.

Could also make the icon slightly smaller so it does not take up that amount of visual space :)

@gastaldi
Copy link
Contributor Author

gastaldi commented May 8, 2018

It would be nice to display it in the newest Checks tab: https://help.github.com/articles/about-status-checks/#checks

@MatousJobanek
Copy link
Contributor

Hi guys,

I finally got to this issue - sorry for the delay.

I'd prefer to update the existing comment instead of adding new comments as this can confuse someone when reviewing the PR (especially if it has a lot of comments)

as for the update of the message hint - we have thee options here:

  1. remove the comment and add a new one
  2. update the current comment
  3. remove the comment and don't add any new one
    I can imagine that adding a new comment can be quite annoying - especially when there is some ongoing discussion, so I would vote for either the second or the third option.

then I would say better to use status api updates rather than comments.

we do use it and we add also details link there to a page that explains what is going on, but nobody has noticed it obviously :-). There are two reasons why we add the comment there.

  1. when there is a PR without a test, we want to do the maximum to force the committer to add a test to the PR.
  2. we can customize the message taking information from the project/configuration file and also by mentioning the committer there

Could also make the icon slightly smaller so it does not take up that amount of visual space :)

It is still possible to customize the message in every project separately by using a config file, so you can define your own message, that will be shorter and without any images.
But if you agree on the image being too big, then I can definitely make it smaller ;-)

It would be nice to display it in the newest Checks tab

First of all, we would have to make the Ike plugins a GH app, then we can use the checks tab. There is an issue for it: #59

@MatousJobanek MatousJobanek self-assigned this Jun 15, 2018
@MatousJobanek
Copy link
Contributor

So, my proposal is:

  • If there is no test, then the comment will contain a smaller image and will look like:

obrazek

  • if a test is added, then the message wiil be modified to:

obrazek

  • if the PR is updated so all changed files are skipped by exclusion, then the message will be modified to:

obrazek

And yeah, the comment will be added only when the PR is submitted/updated by a commit that doesn't contain any test.

@MatousJobanek MatousJobanek modified the milestones: Sprint 150, Sprint 151 Jun 19, 2018
MatousJobanek added a commit to MatousJobanek/ike-prow-plugins that referenced this issue Jun 21, 2018
MatousJobanek added a commit to MatousJobanek/ike-prow-plugins that referenced this issue Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants