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

OddpubMetrics Docker image and Database Table Generation #15

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

quang-ng
Copy link
Collaborator

Working on issue #14

@quang-ng quang-ng marked this pull request as ready for review December 13, 2024 10:32
@joshlawrimore
Copy link
Contributor

@leej3 can you test if the docker image installs oddpub in R in on your machine or @quang-ng can you write a GitHub action that tests it? I cannot build the latest image without the install of oddpub failing on my machine. Thanks!

…hout mock API, ensuring correct data retrieval from the database.
@quang-ng
Copy link
Collaborator Author

@leej3 can you test if the docker image installs oddpub in R in on your machine or @quang-ng can you write a GitHub action that tests it? I cannot build the latest image without the install of oddpub failing on my machine. Thanks!

done, please help me review it

Copy link
Contributor

@leej3 leej3 left a comment

Choose a reason for hiding this comment

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

Thanks @quang-ng, it looks pretty solid! Some details to address but it's almost ready.

.docker/postgres-compose.yaml Outdated Show resolved Hide resolved
services/oddpub/README.md Outdated Show resolved Hide resolved
services/oddpub/README.md Outdated Show resolved Hide resolved
services/oddpub/README.md Outdated Show resolved Hide resolved
services/oddpub/_entrypoint.sh Show resolved Hide resolved
@@ -13,5 +13,13 @@ services:
timeout: 5s # Added missing timeout value
retries: 5

oddpub-dsst:
Copy link
Contributor

Choose a reason for hiding this comment

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

if we add all the services to this file then we should move it to the root directory and rename it to compose.yaml.
We could also launch the container using python-on-whales, or add separate compose files to this directory.

services/oddpub/README.md Show resolved Hide resolved
.mockenv Outdated Show resolved Hide resolved
oddpub_metrics.work_id = self.work_id
oddpub_metrics.document_id = self.document_id
self.db_session.add(oddpub_metrics)
self.db_session.commit()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide some output to the user here to confirm the tool has run correctly and perhaps show the results concisely

Copy link
Contributor

Choose a reason for hiding this comment

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

also, perhaps we should get user confirmation before uploading/updating the database and have a -y flag for those who are sure they want to upload?

# self.session.rollback()

# Check if the Works table exists before attempting to update or delete
inspector = inspect(self.engine)
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we deleting here instead of rolling back?

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