Skip to content

Conversation

smoya
Copy link
Contributor

@smoya smoya commented May 6, 2025

No description provided.

@smoya smoya temporarily deployed to internal-contributors May 6, 2025 12:48 — with GitHub Actions Inactive
Copy link
Collaborator

@jgpruitt jgpruitt left a comment

Choose a reason for hiding this comment

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

These tests were designed to work with the build.py in the projects/pgai/db directory which creates a pgai-ext container. In CI, we use the pgai-ext container for all of these tests. If we want to change any of this, we should consider the whole setup.

@smoya
Copy link
Contributor Author

smoya commented May 6, 2025

These tests were designed to work with the build.py in the projects/pgai/db directory which creates a pgai-ext container.

I see it builds and runs pgai-db instead 🤔

def docker_build() -> None:
"""builds the dev docker image"""
subprocess.run(
" ".join(
[
"docker build",
f"--build-arg PG_MAJOR={pg_major()}",
"--target pgai-lib-db-dev",
"-t pgai-db",
f"--file {ext_dir()}/Dockerfile",
f"{ext_dir()}",
]
),
shell=True,
check=True,
env=os.environ,
text=True,
cwd=ext_dir(),
)

@smoya smoya marked this pull request as ready for review May 6, 2025 13:10
@smoya smoya requested a review from a team as a code owner May 6, 2025 13:10
@jgpruitt
Copy link
Collaborator

jgpruitt commented May 6, 2025

These tests were designed to work with the build.py in the projects/pgai/db directory which creates a pgai-ext container.

I see it builds and runs pgai-db instead 🤔

def docker_build() -> None:
"""builds the dev docker image"""
subprocess.run(
" ".join(
[
"docker build",
f"--build-arg PG_MAJOR={pg_major()}",
"--target pgai-lib-db-dev",
"-t pgai-db",
f"--file {ext_dir()}/Dockerfile",
f"{ext_dir()}",
]
),
shell=True,
check=True,
env=os.environ,
text=True,
cwd=ext_dir(),
)

Weird. CI still references the pgai-ext container. How is this working?

I think we should consider a more comprehensive cleanup of this

@smoya
Copy link
Contributor Author

smoya commented May 6, 2025

Weird. CI still references the pgai-ext container. How is this working?

I think we should consider a more comprehensive cleanup of this

But we use it in CI only for the build-and-test-extension, and the step that runs those are build-and-test-pgai-db-module. In other words, the changes made here don't apply to the extension but rather to the "db" module under /projects/pgai/db

@jgpruitt jgpruitt dismissed their stale review May 6, 2025 16:33

Dismissing my review. Someone else take a look

Copy link
Member

@JamesGuthrie JamesGuthrie left a comment

Choose a reason for hiding this comment

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

I haven't taken a very deep loop at this, but at the surface level it looks right.

I suspect that when we copy-pasted these files from the extension repository when setting up projects/pgai/db, we didn't change pgai-ext to pgai-db.

This isn't surfaced in CI because it runs everything in docker, so these paths are never hit.

@smoya smoya merged commit c08d887 into main May 8, 2025
14 checks passed
@smoya smoya deleted the sergio-fix-testcontainer-for-db branch May 8, 2025 11:23
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