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 GitHub scm source implementation for status checks properties #66

Merged

Conversation

XiongKezhi
Copy link
Contributor

@XiongKezhi XiongKezhi commented Oct 15, 2020

@XiongKezhi XiongKezhi requested a review from a team October 19, 2020 16:29
@XiongKezhi XiongKezhi marked this pull request as ready for review October 19, 2020 16:30
@XiongKezhi XiongKezhi added the enhancement New feature or request label Oct 19, 2020
@XiongKezhi XiongKezhi self-assigned this Oct 19, 2020
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Does this approach work the plain GitSCM as well?

pom.xml Outdated
@@ -25,7 +25,7 @@
<useBeta>true</useBeta>

<!-- Jenkins Plug-in Dependencies Versions -->
<checks-api.version>1.0.2</checks-api.version>
<checks-api.version>2.0.0-SNAPSHOT</checks-api.version>
Copy link
Member

Choose a reason for hiding this comment

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

It would make sense to use the associated incrementals build:
https://repo.jenkins-ci.org/incrementals/io/jenkins/plugins/checks-api/

Comment on lines 31 to 33
/**
* {@inheritDoc}
*/
Copy link
Member

Choose a reason for hiding this comment

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

Such comments can be removed since the IDE is smart enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added more meaningful docs.

.filter(t -> t instanceof GitHubSCMSourceStatusChecksTrait)
.findFirst()
.map(t -> (GitHubSCMSourceStatusChecksTrait)t)
.orElseThrow(IllegalStateException::new);
Copy link
Member

Choose a reason for hiding this comment

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

Are you planning to add a test? It is hard to follow the setup here.
If there is no trait defined then an exception will be thrown, is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, just a problem in my logic. Fixed.

@XiongKezhi
Copy link
Contributor Author

Does this approach work the plain GitSCM as well?

No, I'll implement a trait for GitSCM after this. The logic is mostly the same.

@XiongKezhi XiongKezhi requested a review from uhafner October 21, 2020 15:41
@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #66 into master will increase coverage by 1.73%.
The diff coverage is 94.44%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #66      +/-   ##
============================================
+ Coverage     79.30%   81.03%   +1.73%     
- Complexity      106      121      +15     
============================================
  Files             8       10       +2     
  Lines           343      385      +42     
  Branches         36       37       +1     
============================================
+ Hits            272      312      +40     
- Misses           50       52       +2     
  Partials         21       21              
Impacted Files Coverage Δ Complexity Δ
...hecks/github/GitHubSCMSourceStatusChecksTrait.java 88.23% <88.23%> (ø) 5.00 <5.00> (?)
.../github/GitHubSCMSourceStatusChecksProperties.java 100.00% <100.00%> (ø) 10.00 <10.00> (?)
...s/plugins/checks/github/GitHubChecksPublisher.java 97.36% <0.00%> (+0.49%) 5.00% <0.00%> (ø%)

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 29e43bc...42d4482. Read the comment docs.

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

could you add a screenshot showing the UI for this?

@timja
Copy link
Member

timja commented Oct 21, 2020

Any docs needed?

@XiongKezhi
Copy link
Contributor Author

Any docs needed?

ok, I will add it.

README.md Outdated Show resolved Hide resolved
@@ -23,6 +23,10 @@ This plugin has been installed, alone with the [General API Plugin](https://gith

By listening to the Jenkins builds, this plugin will automatically publish statuses (pending, in progress, and completed) to GitHub.

The status checks can be customized by configuring the "Status Checks Properties" behaviour for you GiHub Source (similar behaviour for Git SCM will be provided soon):

![Status Checks Properties](docs/images/status-checks-properties.png)
Copy link
Member

Choose a reason for hiding this comment

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

might want to fix your spelling of customized in the screenshot

README.md Outdated

The status checks can be customized by configuring the "Status Checks Properties" behaviour for you GiHub Source (similar behaviour for Git SCM will be provided soon):
However, this is not a default feature.
Copy link
Member

Choose a reason for hiding this comment

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

hmm don't we want it to be the default (like it currently is) by just installing this plugin?

Copy link
Contributor Author

@XiongKezhi XiongKezhi Oct 23, 2020

Choose a reason for hiding this comment

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

oh, I mixed up:dizzy_face:, so let me make it clear:

  1. in checks-api, we set the default value of skip to be true, so users won't get logs when no implementation is installed;
  2. in github-checks, we set the default value of skip to be false, so users can get statuses as long as they installed the implementation, even they didn't add the trait at all?

@XiongKezhi XiongKezhi requested a review from timja October 25, 2020 15:43
@XiongKezhi XiongKezhi merged commit 1f04361 into master Oct 25, 2020
@XiongKezhi XiongKezhi deleted the add-scm-source-implementation-for-status-checks-properties branch October 25, 2020 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants