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

[#4181]Improvement: Support setting gradle property skipDockerTests by environment variant #4229

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

featherchen
Copy link
Member

@featherchen featherchen commented Jul 21, 2024

What changes were proposed in this pull request?

Read the environment variable (if exists) when -PskipDockerTests is not specified.

Why are the changes needed?

Fix: #4181

Does this PR introduce any user-facing change?

No

How was this patch tested?

export SKIP_DOCKER_TESTS=false
./gradlew build
# check docket tests are running

@yuqi1129
Copy link
Contributor

Can you please add some description in the doc

+ Gravitino excludes all Docker-related tests by default. To run Docker related tests, make sure you have installed
Docker in your environment and set `skipDockerTests=false` in the `gradle.properties` file (or
use `-PskipDockerTests=false` in the command). Otherwise, all Docker required tests will skip.

@featherchen
Copy link
Member Author

Can you please add some description in the doc

+ Gravitino excludes all Docker-related tests by default. To run Docker related tests, make sure you have installed
Docker in your environment and set `skipDockerTests=false` in the `gradle.properties` file (or
use `-PskipDockerTests=false` in the command). Otherwise, all Docker required tests will skip.

Sure, I have updated the doc.

@yuqi1129
Copy link
Contributor

LTGM
@mchades would you like to take a look?

@xunliu
Copy link
Member

xunliu commented Jul 22, 2024

@yuqi1129 @mchades @featherchen Do we need to keep skipDockerTests propertie in the https://github.com/apache/gravitino/blob/main/gradle.properties#L43 ?

@yuqi1129
Copy link
Contributor

@yuqi1129 @mchades @featherchen Do we need to keep skipDockerTests propertie in the main/gradle.properties#L43 ?

If we do not add it in the gradle.properties, what's the default value of skipDockerTests?

build.gradle.kts Outdated
Comment on lines 693 to 699
val skipDockerTest = if (extra["skipDockerTests"].toString().toBoolean()) {
System.getenv("SKIP_DOCKER_TESTS")?.toBoolean() ?: true
} else {
false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comments to clarify the logic.

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 have add some comments, hope it could help to make the logic clearer.

@featherchen
Copy link
Member Author

@yuqi1129 @mchades @featherchen Do we need to keep skipDockerTests propertie in the main/gradle.properties#L43 ?

If we do not add it in the gradle.properties, what's the default value of skipDockerTests?

Furthermore, if we do not add it, we still need to give the default value of it in related parts of build.gradle.kts

Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

LGTM, do you have more comments? @xunliu @yuqi1129

@yuqi1129
Copy link
Contributor

LGTM, do you have more comments? @xunliu @yuqi1129

I'm okay with the current changes.

@yuqi1129 yuqi1129 merged commit ecf5b08 into apache:main Jul 23, 2024
33 checks passed
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.

[Improvement] Support setting gradle property skipDockerTests by environment variant
4 participants