Compatibility with rails 8.1 #436
Conversation
- Handle `cast_type` being the second parameter for `SpatialColumn.initialize`. - Handle nil case during OID initiation. - Handle case where in `spatial_factory` the object could be frozen for whatever reason. - Update CI to test against 8.1 and supported PG and Rubies.
BuonOmo
force-pushed
the
feat/rails-8-1
branch
from
5078f6b to
37d92d1
Compare
Both test databases where using the same database name, overriding each other.
BuonOmo
force-pushed
the
feat/rails-8-1
branch
from
9e40197 to
89fa8e6
Compare
BuonOmo
force-pushed
the
feat/rails-8-1
branch
3 times, most recently
from
8add7aa to
8c5e033
Compare
BuonOmo
force-pushed
the
feat/rails-8-1
branch
from
cc6bced to
1f1e73f
Compare
BuonOmo
requested review from
keithdoggett and
teeparham
and removed request for
teeparham
| def views | ||
| super - %w[geography_columns geometry_columns] | ||
| end |
There was a problem hiding this comment.
I have a tiny doubt whether we want that behavior, thoughts?
There was a problem hiding this comment.
I think it's probably fine to exclude it. My feel is that this should return the views that the user defines in the app. I'd imagine anything that needs to access these views would not check this first and query them directly so them not being included in this is fine.
Also this is consistent with us excluding spatial_ref_sys from the schema dump IMO.
BuonOmo
force-pushed
the
feat/rails-8-1
branch
from
5499aa5 to
778a833
Compare
|
Hooked up this branch to my app and put it through some paces and ran it through the app's test suite. At least for the features I use, this looks ok to me. |
| def views | ||
| super - %w[geography_columns geometry_columns] | ||
| end |
There was a problem hiding this comment.
I think it's probably fine to exclude it. My feel is that this should return the views that the user defines in the app. I'd imagine anything that needs to access these views would not check this first and query them directly so them not being included in this is fine.
Also this is consistent with us excluding spatial_ref_sys from the schema dump IMO.
| run: | | ||
| # https://www.postgresql.org/support/versioning/ | ||
| pg_targets='[14, 15, 16, 17, 18]' | ||
| tags=$( | ||
| curl -s "https://hub.docker.com/v2/repositories/postgis/postgis/tags?page_size=100" | | ||
| jq --compact-output ' | ||
| .results | ||
| | map(.name) as $tags | ||
| | '"$pg_targets"' | ||
| | map( | ||
| . as $target | ||
| | $tags | ||
| | map(select(test("^\($target)-\\d+\\.\\d+$")))[0] | ||
| ) | ||
| ' | ||
| ) | ||
| if [[ $(jq 'length' <<< "$tags") -ne $(jq 'length' <<< "$tags") ]]; then | ||
| echo "Error: Could not find tags for all PostgreSQL targets" >&2 | ||
| exit 1 | ||
| fi | ||
| echo "postgis_tags=$tags" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
what's the reasoning behind needing to do this instead of an array of versions? is it just because postgis does not have the same versions as postgres in docker?
There was a problem hiding this comment.
Exactly the postgis version vary depending on on the pg version. Here we ensure we are using the latest, yet not the master branch. This is the most likely used by users of this repo IMO. We could also consider adding postgis to the matrix, and just check if version tags exists. Something like:
jobs:
orchestrate:
matrix:
postgres: [14, 15, ..]
postgis: [3.5, 3.6]
output:
tags: ...
run: Check if matrix elements contain tags
There was a problem hiding this comment.
ok got it. Seems fine as if. If there's issues with versioning for some reason later we could move to the tags you suggested but doesn't seem like a problem
|
Merging now! I'll prepare a release tomorrow morning (UTC) :) |
|
@t27duck and others pointing to this branch, I'll delete it once published to rubygem. |
Follow up on the work of @t27duck on #435.
Closes #434
TODO:
18-master, and I'm not sure we want to point to postgis' test branch anyway.