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

[SPARK-31860][BUILD][2.4] only push release tags on success #28667

Conversation

holdenk
Copy link
Contributor

@holdenk holdenk commented May 28, 2020

What changes were proposed in this pull request?

Only push the release tag after the build has finished.

Why are the changes needed?

If the build fails we don't need a release tag.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Running locally with a fake user

@holdenk
Copy link
Contributor Author

holdenk commented May 28, 2020

cc @dongjoon-hyun ?

@holdenk
Copy link
Contributor Author

holdenk commented May 28, 2020

cc @shaneknapp

@SparkQA
Copy link

SparkQA commented May 28, 2020

Test build #123245 has finished for PR 28667 at commit d26c735.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member

cc @dbtsai , too

@holdenk
Copy link
Contributor Author

holdenk commented Jun 1, 2020

cc @srowen & @cloud-fan

@holdenk holdenk changed the title [WIP][SPARK-31860][BUILD] only push release tags on success [SPARK-31860][BUILD] only push release tags on success Jun 1, 2020
@srowen
Copy link
Member

srowen commented Jun 1, 2020

I don't know these scripts well, but yes we should only push on success. This looks sensible.

@holdenk holdenk changed the title [SPARK-31860][BUILD] only push release tags on success [WIP][SPARK-31860][BUILD] only push release tags on success Jun 1, 2020
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jun 1, 2020

Sorry, but I'll close this first to prevent accidental merge on this.
Let's proceed this on master first.

@holdenk
Copy link
Contributor Author

holdenk commented Jun 1, 2020

I've got this for the 2.4 branch since I RMd the current release, I'm annoyed you closed this @dongjoon-hyun.

@dongjoon-hyun dongjoon-hyun reopened this Jun 1, 2020
@dongjoon-hyun
Copy link
Member

This is reopened according to @holdenk 's request.

…rt because when I was doing the 2.4.6 release I found dry_run was still pushing tags so I think something else is broken there but that's a seperate thing to poke at)
@holdenk holdenk changed the title [WIP][SPARK-31860][BUILD] only push release tags on success [WIP][SPARK-31860][BUILD][2.4] only push release tags on success Jun 1, 2020
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.

@holdenk . In any way, I believe this help you complete 2.4.6 release. So, sorry for closing this if this made any delay to you or 2.4.6 release.

BTW, could you make a PR on master and branch-3.0 eventually to prevent any regression at 3.0 after 2.4.6?

@holdenk
Copy link
Contributor Author

holdenk commented Jun 1, 2020

Yeah I'll do a PR to master & branch-3.0 once I've gotten sign off on this being what we want to do (I know you had some reservations when we chatted).

@dbtsai
Copy link
Member

dbtsai commented Jun 1, 2020

It does make sense to push the tag only on success, and let's merge it in 2.4, and then also send out PRs to both master / 3.0. Thanks for working on this!

@holdenk
Copy link
Contributor Author

holdenk commented Jun 1, 2020

Ok cool. I'm going to double check running it locally to make sure it the release still runs ok (I did a run last last week but just to be safe since we don't have any automated testing for these scripts), and once that's done I'll merge this and open a master & 3 PR :)

@SparkQA
Copy link

SparkQA commented Jun 1, 2020

Test build #123393 has finished for PR 28667 at commit d26c735.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@holdenk
Copy link
Contributor Author

holdenk commented Jun 1, 2020

jenkins retest this please

@SparkQA
Copy link

SparkQA commented Jun 1, 2020

Test build #123395 has finished for PR 28667 at commit 9bd1dca.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@holdenk
Copy link
Contributor Author

holdenk commented Jun 1, 2020

Tested build script locally, works ok :)

@holdenk
Copy link
Contributor Author

holdenk commented Jun 1, 2020

(looks like I forgot to push the commit I tested it with, pushed now)

@holdenk holdenk changed the title [WIP][SPARK-31860][BUILD][2.4] only push release tags on success [SPARK-31860][BUILD][2.4] only push release tags on success Jun 1, 2020
@SparkQA
Copy link

SparkQA commented Jun 2, 2020

Test build #123401 has finished for PR 28667 at commit 9bd1dca.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@holdenk
Copy link
Contributor Author

holdenk commented Jun 2, 2020

Only test failure is unit test, unrelated and different than the last test failures. I'm going to merge this to branch-2.4 now. There is a separate PR to master at #28700

asfgit pushed a commit that referenced this pull request Jun 2, 2020
### What changes were proposed in this pull request?

Only push the release tag after the build has finished.

### Why are the changes needed?

If the build fails we don't need a release tag.

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

### How was this patch tested?
Running locally with a fake user

Closes #28667 from holdenk/SPARK-31860-only-push-release-tags-on-success.

Authored-by: Holden Karau <hkarau@apple.com>
Signed-off-by: Holden Karau <hkarau@apple.com>
@@ -24,7 +24,7 @@ function exit_with_usage {
local NAME=$(basename $0)
cat << EOF
usage: $NAME
Tags a Spark release on a particular branch.
Tags a Spark release on a particular branch. Must push after
Copy link
Contributor

Choose a reason for hiding this comment

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

uncompleted sentence Must push after ...?

@SparkQA
Copy link

SparkQA commented Jun 2, 2020

Test build #123403 has finished for PR 28667 at commit dd9e4a4.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor

previously I delete the tags if something goes wrong. This looks like a better solution

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/123403/
Test FAILed.

asfgit pushed a commit that referenced this pull request Jun 2, 2020
### What changes were proposed in this pull request?

Only push the release tag after the build has finished.

### Why are the changes needed?

If the build fails we don't need a release tag.

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

### How was this patch tested?
Running locally with a fake user in #28667

Closes #28700 from holdenk/SPARK-31860-build-master-only-push-tags-on-success.

Authored-by: Holden Karau <hkarau@apple.com>
Signed-off-by: Holden Karau <hkarau@apple.com>
asfgit pushed a commit that referenced this pull request Jun 2, 2020
### What changes were proposed in this pull request?

Only push the release tag after the build has finished.

### Why are the changes needed?

If the build fails we don't need a release tag.

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

### How was this patch tested?
Running locally with a fake user in #28667

Closes #28700 from holdenk/SPARK-31860-build-master-only-push-tags-on-success.

Authored-by: Holden Karau <hkarau@apple.com>
Signed-off-by: Holden Karau <hkarau@apple.com>
(cherry picked from commit 69ba9b6)
Signed-off-by: Holden Karau <hkarau@apple.com>
@holdenk holdenk closed this Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants