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

feat: add dags ci github action #472

Merged
merged 1 commit into from
Aug 23, 2023
Merged

feat: add dags ci github action #472

merged 1 commit into from
Aug 23, 2023

Conversation

bibliotechy
Copy link
Contributor

@bibliotechy bibliotechy commented Jul 27, 2023

Add github workflows for doing basic checking of dags and then deploying them to s3.

Creates a basic dags directory so the tests have something to check

@bibliotechy bibliotechy force-pushed the chad/dags-deployment branch 6 times, most recently from faaeac0 to 3826b01 Compare August 4, 2023 20:47
amywieliczka
amywieliczka previously approved these changes Aug 15, 2023
Copy link
Collaborator

@amywieliczka amywieliczka left a comment

Choose a reason for hiding this comment

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

I'm a bit confused by the re-ordering of import statements. At first it looked like you were grouping built-in modules vs. 3rd-party modules, vs. our own modules, grouping each group by "import " vs. "from ", and then ordering each group alphabetically by module name. But I don't think metadata_mapper/mappers/mapper.py (some of our own modules are grouped with 3rd party modules) or metadata_mapper/mappers/oai/oai_mapper.py (alphabetical sorting with ., and ..mapper) quite follow this sort order. Not a huge deal, but I do want to make sure I understand any order you're asserting here so I can try to maintain it as I move forward.

Also, I've not used a Pipfile or pipenv before. I'm confused by the relationship between dags/Pipfile and dags/requirements.txt. Since we had previously conceived of running these tasks in separate aws lambdas, there's a bit of a proliferation of requirements.txts in this repo. I'm not quite sure if I still see the value in having all of these separate requirements.txts anymore though.

Finally, it seems that universally throughout the codebase, instances of from . import settings need to be replaced with from .. import settings

content_harvester/by_collection.py Outdated Show resolved Hide resolved
content_harvester/by_page.py Outdated Show resolved Hide resolved
content_harvester/tests.py Outdated Show resolved Hide resolved
@bibliotechy bibliotechy force-pushed the chad/dags-deployment branch 3 times, most recently from 8c98e30 to da7e189 Compare August 21, 2023 20:19
@bibliotechy bibliotechy changed the title feat: add dags ci and cd github actions feat: add dags ci github action Aug 21, 2023
Copy link
Collaborator

@amywieliczka amywieliczka left a comment

Choose a reason for hiding this comment

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

Thanks!

@bibliotechy bibliotechy merged commit a211578 into main Aug 23, 2023
1 of 2 checks passed
@amywieliczka amywieliczka deleted the chad/dags-deployment branch February 28, 2024 02: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.

3 participants