Skip to content

Conversation

@syvb
Copy link
Member

@syvb syvb commented Oct 3, 2022

Adds support for binary update testing in GitHub Actions. #554 and #556 need to be merged first.

@syvb syvb mentioned this pull request Oct 3, 2022
5 tasks
Copy link
Contributor

@epgts epgts left a comment

Choose a reason for hiding this comment

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

Thanks for starting this!

- name: Run Update Tests
run: su postgres -c 'sh tools/build -pg$PGVERSION -cargo-pgx /home/postgres/pgx/0.4/bin/cargo-pgx -cargo-pgx-old /home/postgres/pgx/0.2/bin/cargo-pgx test-updates 2>&1'
- name: Run binary update tests
run: OS_NAME=ubuntu OS_VERSION=20.04 su postgres -c 'sh tools/testbin -bindir .. -version 1.11.0 -dbroot /tmp/db -pgport 28814 deb'
Copy link
Contributor

Choose a reason for hiding this comment

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

As I learned this afternoon, we actually run on Debian 10!

My near-term goal after finishing the current batch of release automation work is going to be extracting the rest of the Docker image building logic out of the packaging scripts and into the image-building script I pushed up today and (sadly) moving that script into the private repository so we can build arm64 images too, and then run both our CI here on that image and ALSO build packages on the same image.

Currently the packaging scripts build and discard a Docker image every time! :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure testbin will work as is. I think we need a third run mode (in addition to the existing 'deb' and 'rpm').

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This should work:

su postgres -c 'OS_NAME=debian OS_VERSION=11 tools/testbin -version no -bindir / -dbroot /tmp/db -pgport 28800 ci 2>&1'

Although the chown bug-fix in PR #562 will need to go in first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I forgot to suggest using -pgversions - this is what we want:

# TODO We've always tested only one postgres version here.  We test them all at release
#  time anyway.  If we want to test them here, too, let's make it less slow first.
su postgres -c 'OS_NAME=debian OS_VERSION=11 tools/testbin -pgversions 14 -version no -bindir / -dbroot /tmp/db -pgport 28800 ci 2>&1'

@syvb syvb force-pushed the sv/binary-test-ci branch 2 times, most recently from cadf804 to 6e12e89 Compare October 5, 2022 15:40
@syvb
Copy link
Member Author

syvb commented Oct 5, 2022

Currently this is blocked on #554 and #556 being merged so that we can push a new CI Docker image with the changes needed for this PR.

@syvb syvb force-pushed the sv/binary-test-ci branch 2 times, most recently from 46ac1ba to 10c3fb5 Compare October 5, 2022 20:15
tools/testbin Outdated
select_pg $PG_VERSION
deb=timescaledb-toolkit-postgresql-${PG_VERSION}=${EPOCH}${FROM_VERSION}~${OS_NAME}${OS_VERSION}
$nop sudo apt-get -qq install $deb
[ $1 = deb ] && $nop sudo apt-get -qq install $deb
Copy link
Contributor

Choose a reason for hiding this comment

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

apt-get install is idempotent so there should be no reason to special-case this; did you hit a problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this run (before I changed that line) I got an error:

+ sudo apt-get -qq install timescaledb-toolkit-postgresql-12=1.5.1~debian10
E: Version '1.5.1~debian10' for 'timescaledb-toolkit-postgresql-12' was not found

Copy link
Contributor

Choose a reason for hiding this comment

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

That's because a few days ago when I wrote "we actually run on Debian 10!" I had mixed up bullseye and buster for the 50th time :)

We're actually on Debian 11.

run: |
(
cd extension
cargo pgx install -c /usr/lib/postgresql/12/bin/pg_config
Copy link
Contributor

Choose a reason for hiding this comment

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

That's what 1ffa4e9#diff-018192a8c8e7dea0df3dbbfdc766bfad7a3017167956b9714f8653b5f7644f72R167 is supposed to do - is it not working?

I did admit that was only lightly tested :)

We should fix it rather than trying to work around it, though.

Copy link
Member Author

@syvb syvb Oct 6, 2022

Choose a reason for hiding this comment

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

I didn't realize that testbin was supposed build Toolkit. I added that after seeing this failed run: it had an error since it couldn't find a control file for Toolkit. I think the problem is that test_ci calls deb_start_test which calls start_test which creates test objects before Toolkit is installed. This works if Toolkit is already installed on the system but fails on CI machines which don't already Toolkit installed.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you made the line that installs the old version not run in 'ci' mode; that's why you found no toolkit installed in start_test.

@syvb syvb force-pushed the sv/binary-test-ci branch 3 times, most recently from d8d7e82 to e3bdeee Compare October 6, 2022 19:43
@syvb syvb force-pushed the sv/binary-test-ci branch from e3bdeee to 785f47d Compare October 6, 2022 19:45
@syvb syvb marked this pull request as ready for review October 6, 2022 19:52
@syvb syvb requested a review from epgts October 6, 2022 19:52
@syvb syvb changed the title Binary testing in CI Binary update testing in CI Oct 7, 2022
bors bot added a commit that referenced this pull request Oct 7, 2022
569: Remove incorrect comment in Dockerfile r=Smittyvb a=Smittyvb

bullseye is Debian 11; buster is Debian 10. We're on the latest Debian release. See #552 (comment).

Co-authored-by: Smittyvb <smitty@timescale.com>
@syvb
Copy link
Member Author

syvb commented Oct 7, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 7, 2022

@bors bors bot merged commit ce28e7c into main Oct 7, 2022
@bors bors bot deleted the sv/binary-test-ci branch October 7, 2022 23:25
bors bot added a commit that referenced this pull request Oct 17, 2022
547: Upgrade to pgx 0.5.0 r=Smittyvb a=Smittyvb

Upgrades to pgx `0.5.0-beta.1` (0.5.0 will hopefully be released in 0-7 days). I originally tested on another branch with a lot of code commented out; this branch cherry picks the commits that fix errors but not the ones that comment code out.

To do:
- [x] Fix the three failing tests in `datum_utils.rs` (pgcentralfoundation/pgrx#740)
- [x] Remove workaround for lack of `Copy` on some types
- [x] Make update tester work in CI (#552)
- [x] Change pgx version from 0.5.0-beta.1 to 0.5.0
- [ ] Publish new CI docker image with pgx 0.5.0 right before merging (#571)

Future work:
- Replace `use pgx::*` with `use pgx::prelude::*`
- Use native pgx types instead of `crate::raw`
- Use `#[pg_aggregate]` instead of our own aggregate builder macro

Co-authored-by: Smitty <smitty@timescale.com>
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.

3 participants