Skip to content
This repository has been archived by the owner on Jan 7, 2021. It is now read-only.

Log messages refienement. #184

Merged

Conversation

frenchu
Copy link
Contributor

@frenchu frenchu commented Jun 1, 2018

Pull Request code was tested on running Sonar instance (version 6.7.3). Changes work as expected and they satisfy user requirements.

Closes #183

@@ -213,7 +213,7 @@ private void postIssueComment(PullRequestRef pr,
// check if issue belongs to the Stash diff view
IssueType type = diffReport.getType(path, issueLine, config.issueVicinityRange());
if (type == null) {
LOGGER.info(
LOGGER.debug(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't agree with this being only debug.
When we can not add the comment for the diff view this is mostly likely a misconfiguration of sonar-stash or the whole sonarqube scan.

Could you elaborate more on your reasoning?

Copy link
Contributor Author

@frenchu frenchu Jun 3, 2018

Choose a reason for hiding this comment

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

I thought it is something normal and can happen. I guess the logic is that sonar scan can find issues which weren't introduced in the current pull request, so they aren't in the diff and these issues are excluded.

If the reason is a misconfiguration I would use WARN level here to indicate this is a problem. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@t-8ch any views on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry. yes.
This can normally happen but should not commonly. The most common case is in case of misconfigurations.
It's not clear which level to choose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, understand, so I'll leave it as Info. Thanks for response.

@@ -225,7 +225,7 @@ private void postIssueComment(PullRequestRef pr,
.postCommentLineOnPullRequest(pr, commentContent, path, line, type);

LOGGER
.debug("Comment \"{}\" has been created ({}) on file {} ({})", issueKey, type, path, line);
.info("Comment \"{}\" has been created ({}) on file {} ({})", issueKey, type, path, line);
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Comment \"{}\" has been linked to a Stash task", comment.getId());
if (LOGGER.isInfoEnabled()) {
LOGGER.info("Comment \"{}\" has been linked to a Stash task", comment.getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@frenchu frenchu force-pushed the feature/183-logging-improvement branch from be34fc6 to 1da4fa9 Compare June 11, 2018 19:42
@frenchu frenchu closed this Jun 11, 2018
@frenchu frenchu reopened this Jun 11, 2018
@frenchu
Copy link
Contributor Author

frenchu commented Jun 11, 2018

@t-8ch pull request amended according to our previous discussion.

I kindly request for the merge.

@t-8ch t-8ch merged commit 7ac1bd7 into AmadeusITGroup:master Aug 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants