Skip to content

[SPARK-26554][BUILD] Update release-util.sh to avoid GitBox fake 200 headers #23476

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.

Sign up for GitHub

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

Closed
dongjoon-hyun wants to merge 2 commits into from
Closed

[SPARK-26554][BUILD] Update release-util.sh to avoid GitBox fake 200 headers #23476

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
241c288
[SPARK-26554][BUILD] Update `release-util.sh` to avoid GitBox fake 20…
dongjoon-hyun Jan 6, 2019
3178a69
Add comments
dongjoon-hyun Jan 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion dev/create-release/release-util.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ function fcreate_secure {
}

function check_for_tag {
curl -s --head --fail "$ASF_REPO_WEBUI;a=commit;h=$1" >/dev/null
# Check HTML body messages instead of header status codes. Apache GitBox returns
# a header with `200 OK` status code for both existing and non-existing tag URLs
Copy link
Contributor

@cloud-fan cloud-fan Jan 7, 2019

Choose a reason for hiding this comment

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

as we can push commit to github repo directly, do we still have sync issues between github repo and apache repo?

Copy link
Contributor

@cloud-fan cloud-fan Jan 7, 2019

Choose a reason for hiding this comment

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

if we don't, maybe we can use the github repo to do the release?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 7, 2019

Choose a reason for hiding this comment

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

Right. GitHub repo can be used.

$ curl -s --head --fail "https://github.com/apache/spark/releases/tag/v2.4.0" > /dev/null
$ echo $?
0
$ curl -s --head --fail "https://github.com/apache/spark/releases/tag/v3.0.0" > /dev/null
$ echo $?
22

For me, since last bad experience, I'm pushing to only GitBox. So, I'm not sure whether GitHub to GitBox sync is safe or not in these days.

Copy link
Contributor

@cloud-fan cloud-fan Jan 7, 2019

Choose a reason for hiding this comment

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

GitHub to GitBox sync must be safe otherwise we have problems. At least I've updated my local ASF_REPO to github repo and merge PR to github repo directly.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 7, 2019

Choose a reason for hiding this comment

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

Then, shall I change it back to check header messages against GitHub tag urls? I can do that as a followup PR if you want.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 7, 2019

Choose a reason for hiding this comment

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

Before proceeding that, let me search some from Apache websites.

Copy link
Contributor

@cloud-fan cloud-fan Jan 7, 2019

Choose a reason for hiding this comment

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

Good point on the apache policy concerns. For now I think we only need to use github at this particular place, because gitbox has a mistake about the reader status.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 7, 2019

Choose a reason for hiding this comment

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

Ya. It was just my confusion. My bad. It looks completely okay.

The ASF has a presence on GitHub at https://github.com/apache

People that do not wish to utilize GitHub may continue using their ASF credentials to push code to gitbox.apache.org - we do not mandate use of one of the other.

Previously, github was able to be used at read-only pointers like pom.xml's connection. After GitBox setup, it's okay for read/write pointers for all GitHub users.

For non-GitHub users (maybe BitBucket or GitLab users), we still use the gitbox URL at write-access-required pointer like pom.xml's developerConnection.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. So for a SPARK-26554 follow-up PR, I'll focus only this check_for_tag function at this time. Thank you for clarification.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 7, 2019

Choose a reason for hiding this comment

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

@cloud-fan . I made a followup PR, #23482.

! curl -s --fail "$ASF_REPO_WEBUI;a=commit;h=$1" | grep '404 Not Found' > /dev/null
Copy link
Member

@srowen srowen Jan 6, 2019

Choose a reason for hiding this comment

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

What does the ! do -- make this fail if grep successfully found a 404? does grep -v get the same effect? No need to change it, it's fine, just curious

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jan 7, 2019

Choose a reason for hiding this comment

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

Thank you for review and approval, @srowen . Yes, right. ! does.
We cannot use grep -v which matches all the other lines doesn't have 404 Not Found. grep only returns 1 when no lines are selected.

Copy link
Member

@srowen srowen Jan 7, 2019

Choose a reason for hiding this comment

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

Oh right, duh, wasn't thinking it through

}

function get_release_info {
Expand Down