Adding runner.sh.j2 and zenodo_uploader.py files #9
Conversation
There was a problem hiding this comment.
Thanks, @johngedell. I left a few suggestions and comments on how to tweak the code to be more ready for production (batch mode).
I'd expect the pull request body to contain some output of the scripts, and examples of application of the renderer with the config .yml files to generate the resulting runner.sh scripts. Can it be done?
Also, can you please demonstrate that this script could upload Docker images to all 3 places (quay, dockerhub, ghcr) and provide the resulting links?
Same would be great for Zenodo, at least using their sandbox website, but keeping in mind we need to eventually upload to their production site.
runner.sh.j2
Outdated
| timestamp=$(date +%Y%m%d%H%M%S) | ||
| env_name="nsls2-analysis-2021-1.2" | ||
|
|
||
| {% if -f "${image}.tar.gz" %} |
There was a problem hiding this comment.
These jinja conditions should be used for values based on config files, not the actual state during runtime. I suggest converting this condition to a normal bash if.
runner.sh.j2
Outdated
| {% for v in range(VALUE) %} | ||
| read -p "To which repository would you like to upload? (Enter 'ghcr', 'dockerhub', or 'quay') " REPO | ||
| {% if "REPO" == "ghcr" %} | ||
| echo $GITHUB_TOKEN | docker login ghcr.io --username $GITHUB_USERNAME --password-stdin | ||
| docker tag ${image}:${timestamp} ghcr.io/$GITHUB_USERNAME/${image}:${timestamp} | ||
| docker image push ghcr.io/$GITHUB_USERNAME/${image}:${timestamp} | ||
| {% elif "REPO" == "dockerhub" %} | ||
| echo $DOCKER_PASSWORD | docker login --username $DOCKER_USERNAME --password-stdin | ||
| docker tag ${image}:${timestamp} $DOCKER_REPO | ||
| docker push $DOCKER_REPO | ||
| {% elif "REPO" == "quay" %} | ||
| docker create --name extra-container ${image}:${timestamp} | ||
| echo $QUAY_PASSWORD | docker login quay.io -u $QUAY_USERNAME --password-stdin | ||
| docker commit extra-container quay.io/$QUAY_REPO | ||
| docker push quay.io/$QUAY_REPO | ||
| docker rm extra-container | ||
| docker image rm quay.io/$QUAY_REPO | ||
| {% else %} | ||
| echo "Invalid entry." | ||
| exit 1 | ||
| {% endif %} | ||
| {% endfor %} |
There was a problem hiding this comment.
I suggest reworking it to exclude the jinja conditions, and rather use the .yml config files as input.
There was a problem hiding this comment.
Okay. I missed this when I was initially created the files. I will add this on my list of changes to make and leave unresolved.
runner.sh.j2
Outdated
| read -p "Would like to upload the file to Zenodo? " ZEN | ||
| {% if "ZEN" == "yes" -o "$ZEN" == "y" %} | ||
| python3 -m zenodo_upload | ||
| {% endif %} |
There was a problem hiding this comment.
Same comment as above - use config files to manage that part.
|
I updated the PR body with the checklist: |
There was a problem hiding this comment.
@johngedell, here is my review. I did not see the items that you mentioned yesterday (such as moving runner.sh from conda/ to the top of the dir, moving all template .j2 files into the templates directory, etc.)
Please address my comments, and add the discussed changes.
conda/Dockerfile.j2
Outdated
| ENV PATH="opt/conda/bin/:$PATH" | ||
| ENV env_name="{{env_name}}" | ||
| ENV python_version="{{python_version}}" | ||
| ENV pkg_name="{% if pkg_name %}{{pkg_name}}={{pkg_version}}{% endif %}" |
There was a problem hiding this comment.
What happens if the pkg_name is undefined?
| ENV pkg_name="{% if pkg_name %}{{pkg_name}}={{pkg_version}}{% endif %}" | |
| ENV pkg_name="{% if pkg_name %}{{ pkg_name }}={{ pkg_version }}{% endif %}" |
conda/zenodo_uploader.py
Outdated
| ) | ||
| args = parser.parse_args() | ||
| upload_to_zenodo(args.file_name_to_upload) | ||
| print("Upload Done!") |
There was a problem hiding this comment.
The print needs to have some resulting information, such as the full URL to where it was uploaded.
Co-authored-by: Maksim Rakitin <mrakitin@users.noreply.github.com>
Co-authored-by: Maksim Rakitin <mrakitin@users.noreply.github.com>
Checklist:
runner.shfile based on the config .yml files (use https://github.com/NSLS-II/conda-pack-template/blob/main/configs/n2sn.yml for now, that should produce a small environment .tar.gz file of ~50MB in size, and update as necessary).