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

Add whitelist label option. #444

Merged
merged 6 commits into from
Jan 13, 2017
Merged

Add whitelist label option. #444

merged 6 commits into from
Jan 13, 2017

Conversation

sjstyle
Copy link
Contributor

@sjstyle sjstyle commented Dec 2, 2016

This adds the Whitelist label option to ghprb's advanced options.

  • If both Whitelist and Blacklist label options are entered, it works in 'AND' mode. In other words, PR should not have Blacklist label and Whiltelist label should exist.

  • If nothing is entered in the Whitelist label option, this option is ignored.

I think adding the Whitelist label option can help solve the #210 issue in the future.

  1. LabelField's empty string check problem.

Https://github.com/sjstyle/ghprb-plugin/commit/c54a32cb01d897c207a1f531aa3873f8ab70e044#diff-4b618248c9f9a59094a30a1212e2d172R170

Previously this plug-in did not check for Empty on this line. As a result, when the user enters a setting value, a non-null space is input, so the empty collection is always returned and the Label Check part is activated.

  1. pr.getLabels method problem.

Https://github.com/sjstyle/ghprb-plugin/commit/c54a32cb01d897c207a1f531aa3873f8ab70e044#diff-70706d3124e337b7f717ad901c08d53aL178

Previously this plugin called the pr.getLabels () method. It seems that we would expect only the labels to be returned because of the get prefix, but in fact this method is supposed to do something else.

Https://github.com/kohsuke/github-api/blob/github-api-1.80/src/main/java/org/kohsuke/github/GHPullRequest.java#L130-L133

This is where calling fetchIssue ().

https://github.com/kohsuke/github-api/blob/github-api-1.80/src/main/java/org/kohsuke/github/GHPullRequest.java#L297-L302

It changes this in this part. This changes the object from <api> / {user} / {repo} / pulls to <api> / {user} / {repo} / issues.

I think this is supposed to be the #441 problem.

@benpatterson
Copy link
Member

Hi @sjstyle thanks a lot for the pull request!

Could you also add some tests that go over some of the different conditions/use cases? For example,

  • Whitelist is set and blacklist is not set (and vice versa)
  • Both whitelist and blacklist are set
  • And then various matches of the above for the pull request...

@sjstyle
Copy link
Contributor Author

sjstyle commented Dec 3, 2016

Our team is developing web applications in the company. The team is using the GitHub Pull Request builder for two purposes. (I have created two GitHub Pull Request Projects.)

First, run the test automatically for each pull request to ensure that the test did not fail. It is reasonable to run the test on every commit, and I think it is good because there is no need to leave a comment.

Second, when you open a pull request to get a quick result, Docker Container Build is executed and the URL that can check the result is left as a comment.

During this process, several Pull Requests were created, and Docker Build was executed for every commit, so the URL Comment that the Pull Request Builder left on success came as much as spam. (If we do not leave a URL in the comment, we will not know which URL will allow access to the Container, as there are multiple Docker Servers in our team.)

In most back-end development, there was a desire to have Docker Build run only in pull requests that the "team wants" because it can be checked to see if it was created correctly as a test. However, the function "AS-IS" in the plug-in was all "Blacklist Label", which was the conclusion that it was necessary to explicitly attach a specific label when creating most pull requests.

To improve this inconvenience, I developed "Triggering only whitelist labels" feature and labeled it "Docker Build" so that the Docker Build Trigger responded. This makes it possible for the team to attach a label to only the pull requests that they want to check for results, thereby reducing unnecessary builds and resource consumption.

@sjstyle
Copy link
Contributor Author

sjstyle commented Dec 3, 2016

Oh, I misunderstood. I'll add some test soon.

@sjstyle
Copy link
Contributor Author

sjstyle commented Dec 5, 2016

@benpatterson
I add whitelist label test, whitelist and blacklist label test.

@benpatterson
Copy link
Member

CC @udisch

@sjstyle I suggest you rebase on top of #449. We can focus on getting that done, then we can look at adding a whitelist.

@benpatterson
Copy link
Member

Oh, and instead of rebasing, it might be easier to create a new PR off of that branch. Whatever is easier for you and however comfortable you are with git.

@sjstyle
Copy link
Contributor Author

sjstyle commented Dec 6, 2016

@benpatterson
I just rebased re-implement-label-support-ignorelist branch. (I think it is easier than make new pull request) If you think it is better make new PR rather than rebase, I will make new PR tomorrow. (My local time now is 01:00AM..)

I'll back soon.

@vad1m
Copy link

vad1m commented Dec 12, 2016

hi guys, I'm excited about that feature! Can we add it asap to the plugin and release new version? Looking forward to see it next days 👍

GitHub api fixed on hub4j/github-api@3a66e90.
So, remove force reload pull request logic when it checks label on the pull request.
@sjstyle
Copy link
Contributor Author

sjstyle commented Jan 2, 2017

@benpatterson
I removed check dirty pull request logic and upgrade github-api version.

@benpatterson
Copy link
Member

benpatterson commented Jan 3, 2017 via email

@benpatterson
Copy link
Member

@sjstyle Ok this is coming along. Could you add job DSL support?

CC @udisch as well.

I'm planning on putting everything together in a PR like #455, but neither of you would have access to that branch, so you'd have to develop on one of your forks. Thanks again for the help.

@sjstyle
Copy link
Contributor Author

sjstyle commented Jan 4, 2017

@benpatterson
I have no write access on this repository, so I added some commit in this pull request.
And, I think GitHub is better than Github, So I made new casing commit instead of using yours.
I also added JobDsl support, but I didn't test it. (Our team using only webhook trigger...)

@benpatterson
Copy link
Member

Thanks @sjstyle I'm taking a look.

@benpatterson
Copy link
Member

@sjstyle just pinging. I'm still working on this. I think we'd be better off if the job DSL for labels could accept a list, much like the other white/black lists. That's what I'm working on.

@benpatterson benpatterson merged commit 5af2e04 into jenkinsci:master Jan 13, 2017
@benpatterson
Copy link
Member

Merged as part of #464

Thanks again for the help.

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