Skip to content

Conversation

@jiangzho
Copy link
Contributor

This is a breakdown from #2 : set up gradle as the build-tool for operator

.gitignore Outdated
gradle/wrapper/gradle-wrapper.jar

# IDE specific files/directories #
#######################################
Copy link
Member

Choose a reason for hiding this comment

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

This is irrelevant to Gradle, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Despite not directly related - with plugin idea and eclipse, these files could be created after running init or build task.

Would you suggest adding them with another PR as well ?

build.gradle Outdated
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
assert JavaVersion.current().isJava11Compatible(): "Java 11 or newer is required"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this file.

# limitations under the License.
#
#
group=org.apache.spark.kubernetes.operator
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this part.

settings.gradle Outdated
// specific language governing permissions and limitations
// under the License.

rootProject.name = 'spark-kubernetes-operator'
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this part.


distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-8.6-bin.zip
Copy link
Member

Choose a reason for hiding this comment

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

Please use the latest 8.7.

distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-8.6-bin.zip
networkTimeout=10000
validateDistributionUrl=true
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Lastly, please update LICENSE file for your new addition. Here is the reference from Apache Iceberg project.

https://github.com/apache/iceberg/blob/228fc9b41f99f423d500a236c68216f0203dd42c/LICENSE#L204-L213

@jiangzho jiangzho force-pushed the gradle branch 2 times, most recently from 16c2681 to 236c11b Compare April 17, 2024 21:34
@@ -0,0 +1,17 @@
#
Copy link
Member

Choose a reason for hiding this comment

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

Like gradle/wrapper/gradle-wrapper.properties, please remove this empty line.

LICENSE Outdated

* gradlew and gradle/wrapper/gradle-wrapper.properties

Copyright: 2010-2019 Gradle Authors.
Copy link
Member

Choose a reason for hiding this comment

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

This PR has a conflict on the license year. Please use the YEAR of gradlew instead of this.

build.gradle Outdated
// under the License.

subprojects {
apply plugin: 'idea'
Copy link
Member

Choose a reason for hiding this comment

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

Apache Spark community uses 2-space indentation. Please fix the indentation.

build.gradle Outdated
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
Copy link
Member

Choose a reason for hiding this comment

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

Did you change this line? The space before "License" looks wrong to me.

build.gradle Outdated
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
Copy link
Member

Choose a reason for hiding this comment

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

Let's use the following style. Please copy the following lines from Apache Iceberg .

build.gradle Outdated
apply plugin: 'eclipse'
apply plugin: 'java'
sourceCompatibility = 11
targetCompatibility = 11
Copy link
Member

Choose a reason for hiding this comment

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

Please use Java 17.

networkTimeout=10000
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionSha256Sum=544c35d6bd849ae8a5ed0bcea39ba677dc40f49df7d1835561582da2009b961d
Copy link
Member

Choose a reason for hiding this comment

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

Please move this to the next line of distributionUrl.

settings.gradle Outdated
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this file? Please remove this empty file.

gradlew Outdated
APP_HOME=$( cd "${APP_HOME:-./}" > /dev/null && pwd -P ) || exit

if [ ! -e $APP_HOME/gradle/wrapper/gradle-wrapper.jar ]; then
curl -o $APP_HOME/gradle/wrapper/gradle-wrapper.jar https://raw.githubusercontent.com/gradle/gradle/v8.6/gradle/wrapper/gradle-wrapper.jar
Copy link
Member

Choose a reason for hiding this comment

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

This PR has a conflict on Gradle version.

  • Here: v8.6
  • gradle/wrapper/gradle-wrapper.properties: v8.7

Please fix this line and double-check manually that we are using v8.7.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

I finished the second round review..

Since there are conflicts in the PR, please resolve them and minimize the PR.

settings.gradle Outdated
@@ -0,0 +1,16 @@
// Licensed to the Apache Software Foundation (ASF) under one
Copy link
Member

Choose a reason for hiding this comment

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

BTW, let's exclude all .gradle files by adding a new line here, @jiangzho .

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you so much, @jiangzho .

@dongjoon-hyun
Copy link
Member

Oh, my bad. I didn't check the PR builder on this PR.

dongjoon-hyun added a commit that referenced this pull request Apr 18, 2024
### What changes were proposed in this pull request?

This PR aims to add `gradlew` to `.licenserc.yaml` as a follow-up of
- #4

### Why are the changes needed?

To recover CI.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

Pass the CIs.

### Was this patch authored or co-authored using generative AI tooling?

No,

Closes #5 from dongjoon-hyun/SPARK-47889.

Authored-by: Dongjoon Hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
@dongjoon-hyun dongjoon-hyun mentioned this pull request Apr 23, 2024
@jiangzho jiangzho deleted the gradle branch July 23, 2024 23:05
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