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

fix generate_database action #479

Merged
merged 13 commits into from
Apr 2, 2024
Merged

Conversation

kelle
Copy link
Collaborator

@kelle kelle commented Mar 5, 2024

  • Re-adds the generate_database.py script to generate the database file for the SIMPLE-binary repo.

  • Changed database name from .db to .sqlite.

@kelle
Copy link
Collaborator Author

kelle commented Mar 5, 2024

Getting a username/password error. Not sure if that's because this is a pull requests or not.

@kelle kelle requested a review from dr-rodriguez March 5, 2024 23:13
@dr-rodriguez
Copy link
Collaborator

Getting a username/password error. Not sure if that's because this is a pull requests or not.

Likely yes, I think there are secrets either in this repo or in the binary repo that control how we can push to the binary repo. I don't know if pull requests can access this information.

# More details on trigger events: https://docs.github.com/en/actions/reference/events-that-trigger-workflows
# More details on trigger events:
# https://docs.github.com/en/actions/reference/events-that-trigger-workflows
pull_request: # added for testing
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not do this on a pull_request, as this action essentially builds the binary file for purposes of the website. That should only be done when we are ready to push a change, not when someone opens a pull request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am pretty sure I added this here just so it would run on this PR. in order to test the script.

Comment on lines -56 to +47
source_file: 'SIMPLE.db'
source_file: 'SIMPLE.sqlite'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once this is merged, we will need to go to the binary repo and delete the old db file to avoid confusion.

@dr-rodriguez
Copy link
Collaborator

Recommend avoiding load_astrodb for now, as that re-defines reference tables and such. It's best to have lower-level functions here (ie, direct calls to astrodbkit2) rather than this wrapper.

@kelle
Copy link
Collaborator Author

kelle commented Mar 13, 2024

Recommend avoiding load_astrodb for now, as that re-defines reference tables and such. It's best to have lower-level functions here (ie, direct calls to astrodbkit2) rather than this wrapper.

I disagree. This is exactly the use case for load_astrodb and I'd rather just fix it at the source. This PR is back in Draft mode while I make the needed changes in astrodb_utils.

@kelle
Copy link
Collaborator Author

kelle commented Mar 13, 2024

Waiting on astrodbtoolkit/astrodb_utils#42

@kelle kelle marked this pull request as ready for review April 2, 2024 20:18
@kelle kelle merged commit 45e67c5 into SIMPLE-AstroDB:main Apr 2, 2024
1 check passed
@kelle kelle deleted the fix-generate-db branch April 2, 2024 20:27
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