Skip to content

GH, docker based (reusable) workflows#788

Merged
sandeepd-nv merged 99 commits intonv-legate:branch-23.07from
sandeepd-nv:gh_docker_reusable
Jul 17, 2023
Merged

GH, docker based (reusable) workflows#788
sandeepd-nv merged 99 commits intonv-legate:branch-23.07from
sandeepd-nv:gh_docker_reusable

Conversation

@sandeepd-nv
Copy link
Contributor

No description provided.

@sandeepd-nv sandeepd-nv added in progress category:task PR is a simple task and will not be included in release notes labels Jul 7, 2023
@sandeepd-nv sandeepd-nv self-assigned this Jul 7, 2023
@sandeepd-nv sandeepd-nv mentioned this pull request Jul 10, 2023
@sandeepd-nv sandeepd-nv changed the title Gh docker reusable GH, docker based (reusable) workflows Jul 10, 2023
Copy link
Contributor

@trxcllnt trxcllnt left a comment

Choose a reason for hiding this comment

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

  1. The PR injects secrets as build args, which implicitly embeds them in the final image history. Please mount them as docker secrets instead.
  2. Pulling large image layers from ghcr.io is very slow unless on a network internal to GitHub.
  3. I've fixed a bunch of things in PR #725, and also have upstream fixes in the 23.08 base images. We should coordinate on merging those first, because they should simplify much of this PR.

Copy link
Contributor

@m3vaz m3vaz left a comment

Choose a reason for hiding this comment

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

Spacing and capitalization nits. The naming of the scripts and the functions in the scripts will cause confusion further down the line if there are repeats and if the only difference is - vs _
Consider creating a conda-management-utils file to host all your utility functions and use arguments on your functions
edit: also indentation nits, we should be consistent in the two space indent

case "$1" in
"unit")
echo "Executing unit tests..."
mamba install -y -n "${DEFAULT_CONDA_ENV:-legate}" -c conda-forge pytest pytest-mock ipython jupyter_client
Copy link
Contributor

Choose a reason for hiding this comment

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

at some point we should figure out why we can't use the base environment, but this is ok for now

@@ -0,0 +1,43 @@
#!/usr/bin/env bash

make_conda_env() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Careful about naming. You have a make_conda_env in this file, then you have a make_conda_env in make-conda-env

@m3vaz
Copy link
Contributor

m3vaz commented Jul 17, 2023

@sandeepd-nv what would be the retention period of images on ghcr.io?

@m3vaz m3vaz requested review from m3vaz and trxcllnt July 17, 2023 21:13
@m3vaz
Copy link
Contributor

m3vaz commented Jul 17, 2023

LGTM, please submit when able.

@m3vaz m3vaz dismissed trxcllnt’s stale review July 17, 2023 21:56

Comments have been addressed

@sandeepd-nv sandeepd-nv merged commit 525886b into nv-legate:branch-23.07 Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:task PR is a simple task and will not be included in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants