-
Notifications
You must be signed in to change notification settings - Fork 182
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
chore: Prepare for 0.7.0 development #1404
Conversation
@@ -7,7 +7,7 @@ index d3544881af1..26ab186c65d 100644 | |||
<ivy.version>2.5.1</ivy.version> | |||
<oro.version>2.0.8</oro.version> | |||
+ <spark.version.short>3.4</spark.version.short> | |||
+ <comet.version>0.5.0-SNAPSHOT</comet.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How was this even working? Perhaps this is unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, there was a corresponding incorrect version in setup-spark-builder
:
comet-version:
description: 'The Comet version to use for Spark'
required: true
default: '0.5.0-SNAPSHOT'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also set the comet version dynamically:
../mvnw -nsu -q versions:set-property -Dproperty=comet.version -DnewVersion=${{inputs.comet-version}} -DgenerateBackupPoms=false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is perhaps the best to separate the PR for this 0.6.0 fix otherwise, branch-0.6 is left in a wrong state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed #1405 to fix the issue in branch-0.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kazuyukitanimura I pushed another commit to remove all of the hard-coded Comet version number from GitHub workflows/actions.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1404 +/- ##
=============================================
- Coverage 56.12% 39.32% -16.81%
- Complexity 976 2081 +1105
=============================================
Files 119 265 +146
Lines 11743 61132 +49389
Branches 2251 12962 +10711
=============================================
+ Hits 6591 24040 +17449
- Misses 4012 32587 +28575
- Partials 1140 4505 +3365 ☔ View full report in Codecov by Sentry. |
@kazuyukitanimura I made more changes since you approved - could you take another look? |
@@ -46,7 +42,6 @@ runs: | |||
run: | | |||
cd apache-spark | |||
git apply ../dev/diffs/${{inputs.spark-version}}.diff | |||
../mvnw -nsu -q versions:set-property -Dproperty=comet.version -DnewVersion=${{inputs.comet-version}} -DgenerateBackupPoms=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was hiding the fact that we had an incorrect version in the diff files. Now that this is removed, CI will fail if we do not have the correct version in the diff files, which is good since we want depend on these files having the correct version when running the Spark SQL tests locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loks good
Thanks @kazuyukitanimura |
Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
How are these changes tested?