Skip to content

JS-654 Don't use maven profile for ITS #5224

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

Merged
merged 1 commit into from
Apr 2, 2025
Merged

JS-654 Don't use maven profile for ITS #5224

merged 1 commit into from
Apr 2, 2025

Conversation

vdiez
Copy link
Contributor

@vdiez vdiez commented Mar 20, 2025

@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod bot changed the title Don't use maven profile for ITS JS-654 Don't use maven profile for ITS Mar 20, 2025
@vdiez vdiez force-pushed the remove-its-profile branch 3 times, most recently from 0ae62f7 to dac38f6 Compare March 20, 2025 14:37
@vdiez vdiez marked this pull request as ready for review March 20, 2025 15:00
@vdiez vdiez force-pushed the remove-its-profile branch 2 times, most recently from ae1ad4d to ae76c65 Compare March 20, 2025 15:26
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@vdiez vdiez requested a review from zglicz March 20, 2025 16:10
Copy link
Contributor

@zglicz zglicz left a comment

Choose a reason for hiding this comment

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

Few questions for you, now that I understand maven profiles :)

@@ -126,7 +126,7 @@ plugin_qa_body: &PLUGIN_QA_BODY
qa_script:
- source cirrus-env QA
- source set_maven_build_version $BUILD_NUMBER
- mvn -f its/plugin/pom.xml -Dsonar.runtimeVersion=${SQ_VERSION} ${MVN_TEST} -B -e -V verify surefire-report:report
- mvn -f its/plugin/pom.xml -DskipTests=false -Dsonar.runtimeVersion=${SQ_VERSION} ${MVN_TEST} -B -e -V verify surefire-report:report
Copy link
Contributor

Choose a reason for hiding this comment

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

hm... all of these tasks will depend on the build task and the build task will run the tests. I do not think we should run them again in each of the qa-body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here we override the skipTests=true of the pom.xml, as in qa tasks we do want to run the tests

@@ -56,5 +56,14 @@
</plugin>
</plugins>
</build>
<properties>
<maven.deploy.skip>true</maven.deploy.skip>
<skipTests>true</skipTests>
Copy link
Contributor

Choose a reason for hiding this comment

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

hm... so you do skipTests, why say 'skipTests=false' above? I guess, in the CI, we know that we run after the build so the tests are always ran. But here, perhaps it's worthyto keep the unit-tests before the its run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't want to run the tests as they are actually ITS, we just want to compile the sources in the its folder

<!-- sonar scanner properties -->
<sonar.sources>src/main</sonar.sources>
<sonar.tests>src/test</sonar.tests>
<sonar.javascript.lcov.reportPaths/>
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this here? lcov is only produced based on the unit-test runs and these are completely unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

otherwise it will inherit from parent pom

<!--
its module is added via profile below
-->
<module>its</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

now learning of profiles, why move it in by default? Won't this cause all of our builds to have to go through these always? Otherwise, they were only invoked with mvn build -Pits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by adding this here, it's taken when running the build task to compile sources, without depending on a profile

@zglicz zglicz force-pushed the remove-its-profile branch from ae76c65 to 29926cb Compare April 2, 2025 07:48
Copy link

sonarqube-next bot commented Apr 2, 2025

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@zglicz zglicz merged commit b45086c into master Apr 2, 2025
19 checks passed
@zglicz zglicz deleted the remove-its-profile branch April 2, 2025 08:06
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.

2 participants