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

Issue #12431: Re-enable sonar #12433

Merged
merged 1 commit into from
Nov 22, 2022
Merged

Conversation

nrmancuso
Copy link
Member

@nrmancuso nrmancuso commented Nov 19, 2022

@romani
Copy link
Member

romani commented Nov 19, 2022

@nrmancuso nrmancuso force-pushed the issue-12431 branch 5 times, most recently from fad931d to ddb2bc0 Compare November 20, 2022 15:13
@nrmancuso nrmancuso assigned nrmancuso and unassigned romani Nov 20, 2022
@nrmancuso nrmancuso force-pushed the issue-12431 branch 5 times, most recently from 119a99d to 877e54e Compare November 21, 2022 14:36
# execution on local for non-master:
# SONAR_TOKEN=xxxxxx PR=xxxxxx WERCKER_GIT_BRANCH=xxxxxx ./.ci/wercker.sh sonarqube
if [[ $PR && $PR =~ ^([0-9]*)$ ]]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

Check for $PR is redundant if we update regexp to have at least one character

Comment on lines +579 to +582
SONAR_PR_VARIABLES="-Dsonar.pullrequest.key=$PR_NUMBER"
SONAR_PR_VARIABLES+=" -Dsonar.pullrequest.branch=$PR_BRANCH_NAME"
Copy link
Member Author

Choose a reason for hiding this comment

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

Variable names were not clear, updated these

SONAR_PR_VARIABLES+=" -Dsonar.pullrequest.base=master"
echo "SONAR_PR_VARIABLES: ""$SONAR_PR_VARIABLES"
fi
if [[ -z $SONAR_TOKEN ]]; then echo "SONAR_TOKEN is not set"; sleep 5s; exit 1; fi
Copy link
Member Author

Choose a reason for hiding this comment

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

converted one liner to have better formatting, I can move this to separate commit if need be

export MAVEN_OPTS='-Xmx2000m'
# until https://github.com/checkstyle/checkstyle/issues/11637
# shellcheck disable=SC2086
mvn -e --no-transfer-progress -Pno-validations clean package sonar:sonar $SONAR_PR_VARIABLES \
mvn -e --no-transfer-progress -Pno-validations clean package sonar:sonar \
$SONAR_PR_VARIABLES \
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to resolve shellcheck issue here, but quoting this variable is problematic, as maven interprets it as one argument.

Comment on lines +85 to +87
export PR_NUMBER=$CIRCLE_PR_NUMBER
export PR_BRANCH_NAME=$CIRCLE_BRANCH
export SONAR_API_TOKEN=$SONAR_TOKEN
Copy link
Member Author

Choose a reason for hiding this comment

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

I did this to make sure that we do not have CI-specific details in the script

Comment on lines -157 to -160
# - validate-with-maven-script:
# name: "sonarqube"
# image-name: *custom_img
# command: "./.ci/validation.sh sonarqube"
Copy link
Member Author

Choose a reason for hiding this comment

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

Due to jq installation, context usage, and environment variables, I have moved this to it's own job.

@@ -22,7 +22,7 @@ if [ -z "$CE_TASK_ID" ]; then
exit 1
fi

TASK_URL="$SONAR_SERVER/api/ce/task\?id=$CE_TASK_ID"
Copy link
Member Author

Choose a reason for hiding this comment

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

\ in link was problematic

@nrmancuso nrmancuso assigned romani and unassigned nrmancuso Nov 21, 2022
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Ok to merge

@romani romani assigned rnveach and unassigned romani Nov 21, 2022
.ci/validation.sh Show resolved Hide resolved
.ci/validation.sh Outdated Show resolved Hide resolved
@@ -72,7 +72,28 @@ jobs:
export PR_NUMBER=$CIRCLE_PR_NUMBER
<< parameters.command >>

sonarqube:
docker:
- image: checkstyle/jdk-11-groovy-git-mvn:11.0.13__3.0.9__2.25.1__3.6.3
Copy link
Member

Choose a reason for hiding this comment

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

groovy
3.0.9

https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#setup

groovy (2.4.8) is required to execute

#12432 (comment)

which version ci using ?

Just pointing out I think we need to make these all consistent otherwise it will get confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I have been thinking about having one image for all CI, since we do so many similar things

@romani romani merged commit 5c6d76f into checkstyle:master Nov 22, 2022
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.

None yet

3 participants