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

[Enhance] Auto run nondex #1033

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MyEnthusiastic
Copy link
Contributor

@MyEnthusiastic MyEnthusiastic commented Nov 17, 2023

Background

https://campuswire.com/c/G7A0E96FD/feed/639
https://campuswire.com/c/G7A0E96FD/feed/637
https://campuswire.com/c/G7A0E96FD/feed/624

Change

Optimize : with -fae and -Dskip.rat to power the script to run more tests at a time

Simplified 1
Actually there is no need to go into each sub-module

Simplified 2

In Maven, earlier lifecycle phases are indeed executed automatically before the current one.

for all modules the dependency will be automatically "compiled" first, but the mvn install is important for the single module when we only want small piece of the system to run

Previously, Installing dependency is daunting, also simplification can help people customize, understand, and use more easily. So that every students in the following semester can use this to run their own repo 😆

Clean : misleading README to make it more straightforward, the name and format for the sh file

@@ -16,7 +16,7 @@ runNondex () {
input="modnames"
while IFS= read -r line
do
mvn edu.illinois:nondex-maven-plugin:$nondex_version:nondex -pl :$line -Dlicense.skip=true | tee ./.runNondex/LOGSSS/$line.log
mvn edu.illinois:nondex-maven-plugin:$nondex_version:nondex -pl :$line -Dlicense.skip=true -Drat.skip=true -DlicenseCheck.numUnapprovedLicenses=99999 -fae | tee ./.runNondex/LOGSSS/$line.log
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Do the flags require explicit =true?

  2. Which example needed that numUnapprovedLicenses?

  3. Does the rest of script support the case where multiple modules have a test failure? (Maybe also expand -fae to the full name, so it's easier to read/maintain.)

Copy link
Contributor

@alexbadia1 alexbadia1 Jan 4, 2024

Choose a reason for hiding this comment

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

  1. Do the flags require explicit =true?

The explicit true should not be required, but improves readability by making the purpose of the flags -Dlicense.skip=true -Drat.skip=true as boolean flags

  1. Which example needed that numUnapprovedLicenses?

I don't know, but is a nice to have as a workaround for a project with many dependencies that have unapproved licenses.

  1. Does the rest of script support the case where multiple modules have a test failure? (Maybe also expand -fae to the full name, so it's easier to read/maintain.)

Changing -fae to --fail-at-end might be a good idea for readability.

Can you clarify what you mean by "support" though. The only other maven commands run in the auto-run-nondex/runNondex.sh script are:

mvn install -DskipTests
mvn -Dexec.executable='echo' -Dexec.args='${project.artifactId}' exec:exec -q -fn | tee modnames

@MyEnthusiastic
Copy link
Contributor Author

MyEnthusiastic commented Nov 22, 2023

Simplify

@darko-marinov
Copy link
Contributor

Please squash your changes into one commit. Can your new script be just an option in the old script, to run on all modules or only one?

Add runondex-All

Optimize the script
@MyEnthusiastic
Copy link
Contributor Author

Please squash your changes into one commit. Can your new script be just an option in the old script, to run on all modules or only one?

I squashed them --- thanks for the suggestion --- the old script can run all --- but it was very complex and need python dependency to convert to HTML and did many un-necessary works. The reason I add the new file is to provide a quick start :)

@MyEnthusiastic MyEnthusiastic changed the title Auto run nondex [Enhance] Auto run nondex Nov 29, 2023
@alexbadia1
Copy link
Contributor

alexbadia1 commented Jan 4, 2024

Hi, the status of this PR is a bit unclear. Are there any specific concerns or changes needed before this PR can be accepted?

This PR also seems to be a duplicate of https://github.com/TestingResearchIllinois/idoft/pull/1069/commits, otherwise the changes look good to me.

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.

3 participants