-
Notifications
You must be signed in to change notification settings - Fork 2
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
Dockerization, Poetry, and Github Actions #8
Conversation
The docker, Poetry, and GH Actions pieces look good to me. I'll defer to someone else to comment on the renaming of variables. |
Thanks Tobias! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worked great once I resolved one small issue, noted below. Otherwise, the code and output look good.
docker-compose.yml
Outdated
command: ["--create_csv"] | ||
tty: true | ||
volumes: | ||
- ./output:/output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I ran this, the output
folder that was created was owned by root
. The script then failed to save any files in the folder with:
Traceback (most recent call last):
File "/usr/local/lib/python3.9/runpy.py", line 197, in _run_module_as_main
return _run_code(code, main_globals, None,
File "/usr/local/lib/python3.9/runpy.py", line 87, in _run_code
exec(code, run_globals)
File "/src/ocw_oer_export/cli.py", line 38, in <module>
main()
File "/src/ocw_oer_export/cli.py", line 30, in main
create_csv(source=args.source)
File "/src/ocw_oer_export/create_csv.py", line 190, in create_csv
with open(output_path, "w", newline="", encoding="utf-8") as csv_file:
PermissionError: [Errno 13] Permission denied: '/output/ocw_oer_export.csv'
This is a fairly common issue with Docker. I was able to chown
the folder to be owned by my current user, then run the commands. What I'd recommend here is creating a private
folder, creating an output
folder within that, committing both folders to git, then adding private
to .gitignore
after it's committed. You'll then need to change the output paths to ./private/output/
. This will ensure the folder is created as the current user when the repo is cloned, avoiding this issue. Placing the output
folder in an ignored folder called private
allows the possibility of adding other gitignored folders in the future if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I committed the directories, using a .gitkeep
and then removing it later after adding the directory to .gitignore
. See if it works now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't remove the .gitkeep, because then the directory is empty, and git won't keep it. I'd add a comment in the gitkeep file, too.
@ibrahimjaved12 I'm not seeing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Everything runs great from a fresh state after adding the private folder
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/3328
Closes https://github.com/mitodl/hq/issues/3327
Description (What does it do?)
This PR adds Docker, Poetry, and Github Action to run Unit tests.
Note: We're relying on
docker-compose run
anddocker run
commands instead ofdocker-compose up
becausetqdm
progress bars do not work as expected withdocker-compose up
. They only log when they're at 100% withdocker-compose up
.How can this be tested?