Skip to content
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

WIP -[AB2D-5870] Update postgres container version to 15 #1300

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

Rwolfe-Nava
Copy link
Contributor

@Rwolfe-Nava Rwolfe-Nava commented Dec 12, 2023

🎫 Ticket

https://jira.cms.gov/browse/AB2D-5870

🛠 Changes

Updates testcontainers and docker image to use postgres 15.
Installs pg_cron inside of the docker images and testcontainers.

ℹ️ Context for reviewers

(Background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers.)

✅ Acceptance Validation

(How were the changes verified? Did you fully test the acceptance criteria in the ticket? Provide reproducible testing instructions and screenshots if applicable.)

🔒 Security Implications

  • This PR adds a new software dependency or dependencies.
  • This PR modifies or invalidates one or more of our security controls.
  • This PR stores or transmits data that was not stored or transmitted before.
  • This PR requires additional review of its security implications for other reasons.

If any security implications apply, add Jason Ashbaugh (GitHub username: StewGoin) as a reviewer and do not merge this PR without his approval.

@Rwolfe-Nava Rwolfe-Nava force-pushed the ab2d-5870-update-postgres-image branch from b1678ed to 3854174 Compare January 5, 2024 17:09
@Rwolfe-Nava Rwolfe-Nava changed the title [AB2D-5870] Update postgres container version to 15 WIP -[AB2D-5870] Update postgres container version to 15 Jan 5, 2024
@Rwolfe-Nava Rwolfe-Nava force-pushed the ab2d-5870-update-postgres-image branch from 7bfa908 to 7586339 Compare January 5, 2024 19:44
.run("echo \"pg_ctl restart\"\n >> /docker-entrypoint-initdb.d/init_pg_cron.sh")
.run("chmod +x /docker-entrypoint-initdb.d/init_pg_cron.sh")
.run("apt-get install -y curl")
.run("apt-get install -y postgresql-15-cron")
Copy link
Member

Choose a reason for hiding this comment

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

Can these lines be moved to the postgres Dockerfile, then that file be referenced with withFileFromPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure. I originally attempted to have init_pg_cron.sh already exist inside the repo, and then just copy it to the container with a:

.copy("<path_to_init_pg_cron.sh>", "/docker-entrypoint-initdb.d/");

The problem I ran into was a null pointer exception with reference to the init_pg_cron.sh path. I tried both a relative path and an absolute path but neither worked. I am probably missing something, but for testing purposes I gave up and just created the script inside the container on the fly using the commands above.

Dockerfile Outdated Show resolved Hide resolved
api/Dockerfile Outdated Show resolved Hide resolved
Copy link
Member

@gsf gsf left a comment

Choose a reason for hiding this comment

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

Coming back to this, I worry this will have a big impact on tests -- will this result in dozens of image builds during testing? Could the postgres image instead be built and uploaded to ECR, then referenced there?

@Rwolfe-Nava
Copy link
Contributor Author

Coming back to this, I worry this will have a big impact on tests -- will this result in dozens of image builds during testing? Could the postgres image instead be built and uploaded to ECR, then referenced there?

I think it already does spin up dozens of images during testing. At least locally, when we run testcontainers it spins up a LOT of containers for all of the tests that run. In that regard, this PR doesn't change that so it should have the same impact that it currently does.

That said, I am not sure what is meant by building the image and uploading it to ECR (I don't know what ECR is so I would need to research this). If that would eliminate the current behavior of spinning up a lot of dummy test-containers that would be a great benefit in my opinion.

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.

2 participants