-
-
Notifications
You must be signed in to change notification settings - Fork 530
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 images base URL property #150
Conversation
@don-vip please take a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also document the property in README. I removed it, it must be restored.
Thanks for review! Instead of previous approach I made new property configurable via server UI with appropriate field description: https://github.com/mc1arke/sonarqube-community-branch-plugin/pull/150/files#diff-6b77d55ef12cf09f58f256a700f17effR93-R97 But you are right, maybe an explanation in the README could be useful to get the clue about new property usage. |
Hi @Brialius @don-vip |
Hey @4n4n4s Pls notice that there is no trailing slash in base URL |
Maybe we should remove it in case someone configures it with a slash to prevent misconfiguration |
Add tests for getBaseImageUrl()
@4n4n4s |
@don-vip property description is added to README |
@mc1arke could you please take a look at this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great contribution. I'm generally happy with your changes, but want a discussion on the points I've raised before merging.
...ain/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/PullRequestPostAnalysisTask.java
Show resolved
Hide resolved
ef82deb
to
9dccee6
Compare
Fix #130