-
Notifications
You must be signed in to change notification settings - Fork 75
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
Docker image #53
Docker image #53
Conversation
For context reference, the reason that we can't build hydra using pgxman as part of its CI is due to the intricate limitation of pgxman's publication mechanism. It necessitates the existence and publication of the buildkit file in a central repo pgxman/buildkit, which in turn refers to a tag for source download. This creates a circular dependency, where the tagging of an extension requires pgxman to build the buildkit artifacts, but the build artifacts require the tag to exist first. This can be worked around now by:
When we can self-publish extensions from individual repo in the future, the circular dependency will be broken, and this workaround will no longer be necessary. |
heya @owenthereal, sorry for the lack of context, I added you here in part because of pgxman but also because I'm a bit of a noob when it comes to Docker so I wanted you to check my work, if you had any suggestions for improvement. For instance I'm current creating a I do think it would be possible to build with pgxman based on the docs here, as you suggest a buildkit yaml would be needed as well. IMO this would just be for making "dev builds" as desired, though for local testing the duckdb build takes a long time and the ccache setup seems to not work super well, so it's not a great local developer tool. |
You could run a specific target with https://docs.docker.com/reference/cli/docker/image/build/#target, e.g.
Not saying we should replace this Docker build with pgxman build right now, but being able to specify cache dir would be a nice pgxman feature to add in the future. |
a811b49
to
d9167ef
Compare
d9167ef
to
d2fdfec
Compare
d2fdfec
to
26d23b4
Compare
this is what ultimately worked for me:
|
I would love to install pg_duckdb in our Postgres Docker image. I tried the Dockerfile from above, but it was stuck at 100% at |
Took a very long time to run for me too. Might have been more than 30 minutes before it was complete. |
Thanks @mike-luabase, that's good to know. |
To make the build faster it would help a lot if you changed the |
c38f089
to
4419031
Compare
Even after #211, there remains an issue with
This is despite the fact we are using |
7d442d8
to
0801d3f
Compare
this doesn't push the image anywhere yet, but this will apparently get around rate limit issues we are seeing
0aa3050
to
7205ee4
Compare
7205ee4
to
41a92a5
Compare
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.
Looks good. We will need a green build to merge though no?
*.cache-to=type=gha,mode=max | ||
*.cache-from=type=gha | ||
postgres.tags=pgduckdb/pgduckdb:${{ matrix.postgres }}-${{ github.sha }} | ||
${{ !contains(github.ref_name, '/') && format('postgres.tags=pgduckdb/pgduckdb:{0}-{1}', matrix.postgres, github.ref_name) || '' }} |
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.
Will this override the one above or add it? Also instead of skipping when branch name have a /
could we replace with -
? (seems arbitrary to just exclude these no?)
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.
It adds to the one above it, so we will push with the sha anytime it gets built, including PRs.
Mm yeah we could replace /
with -
, but this was sorta an accidental "feature" -- by doing it this way we only get tags and main
pushed as "named tags" rather than PRs.
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.
In that case, might as well be explicit cause if I name my branch hello
it would be pushed but not yl/hello
:-)
${{ !contains(github.ref_name, '/') && format('postgres.tags=pgduckdb/pgduckdb:{0}-{1}', matrix.postgres, github.ref_name) || '' }} | |
${{ github.ref_name == "main" && format('postgres.tags=pgduckdb/pgduckdb:{0}-main', matrix.postgres) || '' }} |
I'm fine either way!
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.
That won't work for tags though (v0.1.0
).
The / will show up in the ref_name for any PR, even if you don't put a / in your branch name.
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.
Looks good. We will need a green build to merge though no?
f3d772b
to
3f29ff6
Compare
Yeah, looks like the azure ubuntu mirror is broken atm. |
This reverts commit 6063ac8. This did not work because this is the initdb directory, which needs to not exist at the time the container is started. We'll need to set the config another way.
PR description updated. I'd like to get this merged here then continue on in separate PRs with docker-compose, docs changes, etc. Especially since there won't be an image tagged with |
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.
Overall this looks good. I left one comment, but that can also be done later. Feel free to merge as you see fit.
|
||
# A more selective copy might be nice, but the git submodules are not cooperative. | ||
# Instead, use .dockerignore to not copy files here. | ||
COPY . . |
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 think it would be good to automatically run CREATE EXTENSION pg_duckdb
and configure shared_preload_libraries=pg_duckdb
. That'll make it easier to run.
Using an initialization script should make automating that possible I think: https://github.com/docker-library/docs/blob/master/postgres/README.md#initialization-scripts
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.
yeah we can handle that at the docker compose level
pgduckdb/pgduckdb
17-$sha
,16-$sha
,15-$sha
17-main
,16-main
,15-main
v
) will be built automatically and pushed to17-$tag
,16-$tag
,15-$tag
To test, grab a tag, for instance:
Then:
Cleanup:
This can be made easier with a
docker-compose.yml
, which will be forthcoming.