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

test lift #244

Merged
merged 25 commits into from
Apr 4, 2023
Merged

test lift #244

merged 25 commits into from
Apr 4, 2023

Conversation

franvila
Copy link
Contributor

@franvila franvila commented Mar 30, 2023

Type of change

  • Enhancement / new feature

Description

In order to have a build with no errors in Kroxylicious we would need to avoid java 19 jdk (integrationtests module) because there is no current support in Sonatype for that version. The LTS supported is JDK 17.

Additional Context

Why are you making this pull request?

scripts/setup.sh Outdated Show resolved Hide resolved
scripts/setup.sh Outdated Show resolved Hide resolved
@k-wall k-wall marked this pull request as ready for review March 31, 2023 08:03
@k-wall
Copy link
Contributor

k-wall commented Mar 31, 2023

@franvila I clicked ready for review by mistake and couldn't find a way to switch it back.

@k-wall
Copy link
Contributor

k-wall commented Mar 31, 2023

The only thing that I don't know how to skip is to avoid running ./mvnw by default when it is present in the project,

Looks like you could config what mvnw does by having your setup script create $HOME/.mavenrc (https://github.com/franvila/kproxy/blob/lift/mvnw#L43). I think you could use it to set MAVEN_CONFIG to -pl !:integrationtests --also-make-dependents.

scripts/setupLift.sh Outdated Show resolved Hide resolved
scripts/setupLift.sh Outdated Show resolved Hide resolved
.lift.toml Outdated Show resolved Hide resolved
@franvila
Copy link
Contributor Author

@franvila I clicked ready for review by mistake and couldn't find a way to switch it back.

Once it is reviewed it cannot be switched back. This PR was intentionally created in draft to not merge it. The original one is that one from Sam, #242

@k-wall k-wall self-requested a review March 31, 2023 10:32
Copy link
Contributor

@k-wall k-wall left a comment

Choose a reason for hiding this comment

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

looks reasonable to me.

@franvila
Copy link
Contributor Author

The only thing that I don't know how to skip is to avoid running ./mvnw by default when it is present in the project,

Looks like you could config what mvnw does by having your setup script create $HOME/.mavenrc (https://github.com/franvila/kproxy/blob/lift/mvnw#L43). I think you could use it to set MAVEN_CONFIG to -pl !:integrationtests --also-make-dependents.

@k-wall tested but it does not work. It seems setup and ./mvnw are executed in parallel so they are not always executed in the same order and does not affect the other.

@k-wall
Copy link
Contributor

k-wall commented Mar 31, 2023

How about ignoring mvnw files in our tree with a ignoreFiles config setting? Like this:

https://community.sonatype.com/t/dependency-tab-using-maven-should-be-gradle/10323/3?u=kwall

@franvila
Copy link
Contributor Author

@k-wall @SamBarker My intention was not to merge this PR, but if you agree we can merge this PR and close Sam's #242

@franvila
Copy link
Contributor Author

franvila commented Mar 31, 2023

How about ignoring mvnw files in our tree with a ignoreFiles config setting? Like this:

https://community.sonatype.com/t/dependency-tab-using-maven-should-be-gradle/10323/3?u=kwall

I tried that as well, but nothing. It seems it is configured by default to detect mvnw files and .lift.toml and execute the process in parallel if they files are found, without ignoring any of those if the other is found.

Anyway, it is "only" noise, non time consuming as they are executed in parallel, we just need one compilation to be able to run the static analysis.

@k-wall
Copy link
Contributor

k-wall commented Mar 31, 2023

How about ignoring mvnw files in our tree with a ignoreFiles config setting? Like this:
https://community.sonatype.com/t/dependency-tab-using-maven-should-be-gradle/10323/3?u=kwall

I tried that as well, but nothing. It seems it is configured by default to detect mvnw files and .lift.toml and execute the process in parallel if they files are found, without ignoring any of those if the other is found.

Anyway, it is "only" noise, non time consuming as they are executed in parallel, we just need one compilation to be able to run the static analysis.

You are right, it probably doesn't matter as the build is reasonable quick. I suppose setup script could run.

rm -f mvnw*

@franvila
Copy link
Contributor Author

rm -f mvnw*

Tested in this commit with no luck https://lift.sonatype.com/results/github.com/kroxylicious/kroxylicious/01GWVQP2KNRBCHQN9YMXRVYE55/?ph=PhaseIntroduced&tab=logs.

As I suspected, both processes are run in parallel without knowing each other, so no matter what the setup does, if mvnw exists at the beginning it will be launched by default.

Copy link
Contributor

@k-wall k-wall left a comment

Choose a reason for hiding this comment

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

Please remember to squash and give it a sensible commit message

@franvila franvila merged commit c6a7609 into kroxylicious:main Apr 4, 2023
@franvila franvila deleted the lift branch April 4, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants