-
Notifications
You must be signed in to change notification settings - Fork 10
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
GH-21: Add release CI #122
Conversation
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.
Some minor comments but looks good! Great work @kou I've tested the verification script locally against the uploaded RC to your fork and have validated successfully.
const PkgVersion = "18.0.0-SNAPSHOT" | ||
const PkgVersion = "18.0.0" |
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 doesn't have to change for this PR, only when releasing, right? or do we want this to be 17.0.0
and increment on the new release?
I think we should probably continue using SNAPSHOT
and just remove the SNAPSHOT
bit when releasing.
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 doesn't have to change for this PR, only when releasing, right?
No.
or do we want this to be 17.0.0 and increment on the new release?
No.
My suggestions:
- Don't use
-SNAPSHOT
suffix for unreleased version- I think that it's needless because we don't create a nightly (or something) package from main
- I think that the
-SNAPSHOT
suffix name is Maven convention: https://maven.apache.org/maven-release/maven-release-plugin/usage/prepare-release.html- If we want to use
-XXX
suffix for unreleased version, other name such as-dev
is better
- If we want to use
- We don't update this just after we release a new version
- Because we can't know the next version (
18.0.1
?18.1.0
?19.0.0
?) just after we release a new version
- Because we can't know the next version (
- We update this just before we release a new version, when we add a new compatible feature (minor version up) or when we add an incompatible change (major version up)
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.
sounds good to me to update just before releasing
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.
just a few nitpicks from me
.github/workflows/rc.yml
Outdated
sed \ | ||
-e 's/^const PkgVersion = "//' \ | ||
-e 's/"$//') | ||
rc=100 |
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.
why rc=100
?
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 just want to use RC that is never used for normal RCs.
We can use 1000
, $(date %Y%m%d)
or something instead of 100
here.
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, gotcha. $(date %Y%m%d)
is probably best i think?
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.
OK. I'll use it.
go clean -modcache || : | ||
if [ "${VERIFY_SUCCESS}" = "yes" ]; then | ||
rm -rf "${VERIFY_TMPDIR}" | ||
else | ||
echo "Failed to verify release candidate. See ${VERIFY_TMPDIR} for details." | ||
fi |
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.
instead of calling go clean -modcache
we could try just setting the GOCACHE
and GOMODCACHE
env vars to point to directories in the tmpdir. That way we still get clean caches, but we don't blow up the users cache dirs.
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.
Removing cache isn't the main purpose of it.
It's for avoiding a permission problem. See also: apache/arrow#38222
BTW, do we need to set GOCACHE
/GOMODCACHE
explicitly when we set GOPATH
? Can GOCACHE
/GOMODCACHE
be set automatically under GOPATH
?
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 gotcha.
BTW, do we need to set
GOCACHE
/GOMODCACHE
explicitly when we setGOPATH
? CanGOCACHE
/GOMODCACHE
be set automatically underGOPATH
?
GOMODCACHE
should generally be ${GOPATH}/pkg/mod
, we might not need to change it explicitly since we set GOPATH
.
GOCACHE
can be anywhere though and typically is set to ~/.cache/go-build
, if we're not running go clean -cache
or otherwise cleaning the build cache, and only using go clean -modcache
then we're probably fine here without issue.
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.
OK. Let's use go clean -modcache
here.
dev/release/verify_rc.sh
Outdated
if [ -d "${TOP_SOURCE_DIR}/arrow-testing/data" ]; then | ||
cp -a "${TOP_SOURCE_DIR}/arrow-testing" "${ARCHIVE_BASE_NAME}/" | ||
else | ||
git clone \ | ||
https://github.com/apache/arrow-testing.git \ | ||
"${ARCHIVE_BASE_NAME}/arrow-testing" | ||
fi | ||
if [ -d "${TOP_SOURCE_DIR}/parquet-testing/data" ]; then | ||
cp -a "${TOP_SOURCE_DIR}/parquet-testing" "${ARCHIVE_BASE_NAME}/" | ||
else | ||
git clone \ | ||
https://github.com/apache/parquet-testing.git \ | ||
"${ARCHIVE_BASE_NAME}/parquet-testing" | ||
fi | ||
|
||
ARROW_TEST_DATA="${ARCHIVE_BASE_NAME}/arrow-testing/data" | ||
export ARROW_TEST_DATA | ||
PARQUET_TEST_DATA="${ARCHIVE_BASE_NAME}/parquet-testing/data" | ||
export PARQUET_TEST_DATA | ||
} |
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.
Need to add PARQUET_TEST_BAD_DATA
now as per #124
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.
OK. I'll add it.
Fix apacheGH-21 This is based on https://github.com/apache/arrow-julia/tree/main/dev/release and https://github.com/apache/arrow-adbc/tree/main/dev/release . Workflow: Cut a RC: 1. Update `PkgVersion` in `arrow/doc.go` 2. Run `dev/release/release_rc.sh` 1. `dev/release/release_rc.sh` pushes `v${version}-rc${rc}` tag 2. `.github/workflows/rc.yml` creates `apache-arrow-go-${version}.tar.gz{,.sha256,.sha512}` 3. `.github/workflows/rc.yml` uploads `apache-arrow-go-${version}.tar.gz{,.sha256,.sha512}` to GitHub Releases 4. `dev/release/release_rc.sh` downloads `apache-arrow-go-${version}.tar.gz` from GitHub Releases 5. `dev/release/release_rc.sh` signs `apache-arrow-go-${version}.tar.gz` as `apache-arrow-go-${version}.tar.gz.asc` 6. `dev/release/release_rc.sh` uploads `apache-arrow-go-${version}.tar.gz.asc` to GitHub Releases 3. Start a vote (GitHub Actions instead of https://dist.apache.org/repos/dist/dev/arrow/ is used like ADBC.) Verify a RC: 1. Run `dev/release/verify_rc.sh` Release an approved RC: 1. Run `dev/release/release.sh` 1. `dev/release/release.sh` pushes `v${version}` tag that refers that same commit ID as `v${version}-rc${rc}` 2. `dev/release/release.sh` downloads `apache-arrow-go-${version}.tar.gz{,.asc,.sha256,.sha512}` from GitHub Actions 3. `dev/release/release.sh` uploads `apache-arrow-go-${version}.tar.gz{,.asc,.sha256,.sha512}` to https://dist.apache.org/repos/dist/release/arrow 4. `dev/release/release.sh` removes old releases from https://dist.apache.org/repos/dist/release/arrow 2. Add this release to ASF's report database: https://reporter.apache.org/addrelease.html?arrow Follow-up tasks: * Add support for running integration test in the verification script * Add support for releasing a release note * Add support for creating "v${version}"'s GitHub Releases * We can copy "v${version}-rc${rc}"'s GitHub Releases and adjust it for the official release
Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
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.
Thanks @kou
with: | ||
submodules: recursive | ||
- name: Prepare for tag | ||
if: github.ref_type == 'tag' |
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.
should this be ${{ github.ref_type }}
?
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 can omit ${{ ... }}
: https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idif
When you use expressions in an if conditional, you can, optionally, omit the ${{ }} expression syntax because GitHub Actions automatically evaluates the if conditional as an expression. However, this exception does not apply everywhere.
You must always use the ${{ }} expression syntax or escape with '', "", or () when the expression starts with !, since ! is reserved notation in YAML format.
I'll merge this. |
Fix GH-21
This is based on
https://github.com/apache/arrow-julia/tree/main/dev/release and https://github.com/apache/arrow-adbc/tree/main/dev/release .
Workflow:
Cut a RC:
PkgVersion
inarrow/doc.go
dev/release/release_rc.sh
dev/release/release_rc.sh
pushesv${version}-rc${rc}
tag.github/workflows/rc.yml
createsapache-arrow-go-${version}.tar.gz{,.sha256,.sha512}
.github/workflows/rc.yml
uploadsapache-arrow-go-${version}.tar.gz{,.sha256,.sha512}
to GitHub Releasesdev/release/release_rc.sh
downloadsapache-arrow-go-${version}.tar.gz
from GitHub Releasesdev/release/release_rc.sh
signsapache-arrow-go-${version}.tar.gz
asapache-arrow-go-${version}.tar.gz.asc
dev/release/release_rc.sh
uploadsapache-arrow-go-${version}.tar.gz.asc
to GitHub Releases(GitHub Actions instead of
https://dist.apache.org/repos/dist/dev/arrow/ is used like ADBC.)
Verify a RC:
dev/release/verify_rc.sh
Release an approved RC:
dev/release/release.sh
dev/release/release.sh
pushesv${version}
tag that refers that same commit ID asv${version}-rc${rc}
dev/release/release.sh
downloadsapache-arrow-go-${version}.tar.gz{,.asc,.sha256,.sha512}
from GitHub Actionsdev/release/release.sh
uploadsapache-arrow-go-${version}.tar.gz{,.asc,.sha256,.sha512}
to https://dist.apache.org/repos/dist/release/arrowdev/release/release.sh
removes old releases from https://dist.apache.org/repos/dist/release/arrowFollow-up tasks: