Skip to content

Resolves ECR_CACHE_URL from env or AWS SSM#4896

Merged
myurasov-nv merged 11 commits intoisaac-sim:developfrom
myurasov-nv:my-ci-ecr-fix-1
Mar 10, 2026
Merged

Resolves ECR_CACHE_URL from env or AWS SSM#4896
myurasov-nv merged 11 commits intoisaac-sim:developfrom
myurasov-nv:my-ci-ecr-fix-1

Conversation

@myurasov-nv
Copy link
Collaborator

@myurasov-nv myurasov-nv commented Mar 9, 2026

Use env vars available on the runner or AWS SSM store to retrieve ECR_CACHE_URL to be flexible with ECR location. Allows for ECR settings to be configured without code change from AWS console.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • [NA] I have added tests that prove my fix is effective or that my feature works
  • [NA] I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@myurasov-nv myurasov-nv requested a review from nv-apoddubny March 9, 2026 21:09
@github-actions github-actions bot added bug Something isn't working documentation Improvements or additions to documentation infrastructure labels Mar 9, 2026
@myurasov-nv myurasov-nv marked this pull request as draft March 9, 2026 21:51
@myurasov-nv myurasov-nv changed the title Uses ECR_* env vars available on the runner Resolves ECR_CACHE_URL from env or AWS SSM Mar 9, 2026
@myurasov-nv myurasov-nv marked this pull request as ready for review March 9, 2026 23:49
@myurasov-nv myurasov-nv marked this pull request as draft March 10, 2026 00:00
@myurasov-nv myurasov-nv marked this pull request as ready for review March 10, 2026 00:00
@isaac-sim isaac-sim deleted a comment from greptile-apps bot Mar 10, 2026
@myurasov-nv myurasov-nv marked this pull request as draft March 10, 2026 00:03
@myurasov-nv myurasov-nv marked this pull request as ready for review March 10, 2026 00:03
@isaac-sim isaac-sim deleted a comment from greptile-apps bot Mar 10, 2026
@myurasov-nv myurasov-nv marked this pull request as draft March 10, 2026 00:27
@myurasov-nv myurasov-nv marked this pull request as ready for review March 10, 2026 00:28
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 10, 2026

Greptile Summary

This PR replaces the static ecr-repository + aws-region inputs with a flexible ECR URL resolution chain: explicit ecr-url input → ECR_CACHE_URL runner env var → SSM parameter looked up via the EC2 Instance Metadata Service (IMDSv2) → graceful local-only fallback. The build.yaml workflow is updated to drop the now-removed inputs from all six action invocations, and the README is trimmed to reflect the new behaviour.

Key changes:

  • New ecr-url input is correctly passed through the env: block (INPUT_ECR_URL) rather than via inline ${{ inputs.ecr-url }} interpolation, addressing shell-injection concerns.
  • IMDSv2 is used for the EC2 instance-ID lookup (fetches a session token first), replacing the previous IMDSv1-only approach.
  • A URL-format validation guard (exit 1 when the region can't be extracted from the hostname) provides a clear error message for malformed URLs, including the China-partition and VPC-endpoint cases.
  • One style-level concern: aws ssm get-parameter is invoked without --region, relying on SDK auto-discovery. When the SSM parameter is stored in a different region than the runner's default, the call fails silently with only a misleading "parameter not found" message.

Confidence Score: 4/5

  • PR is safe to merge; the new ECR URL resolution logic is well-structured with appropriate fallbacks and a URL format validation guard.
  • The PR introduces sound changes to CI infrastructure: dynamic ECR URL resolution via input → env var → IMDSv2-based SSM lookup with graceful fallback. The implementation correctly uses the env: block for input interpolation (addressing shell-injection risks) and validates URL formats. The only remaining concern is cross-region SSM parameter support — the action currently assumes the SSM parameter is in the same region as the runner, and if not, it fails silently with a misleading error message. This is a design limitation rather than a functional regression, and the fallback behavior (building locally) remains graceful.
  • .github/actions/ecr-build-push-pull/action.yml — SSM region handling (consider adding explicit --region to the SSM call or exposing it as a configurable parameter).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([Start: ECR Build-Push-Pull Action]) --> B{ecr-url input\nset?}
    B -- Yes --> G[Use INPUT_ECR_URL]
    B -- No --> C{ECR_CACHE_URL\nenv var set?}
    C -- Yes --> G
    C -- No --> D[Fetch IMDSv2 token\nvia PUT /latest/api/token]
    D --> E{Instance ID\nresolved from IMDS?}
    E -- No --> F[🟠 Log: Not on EC2 / IMDS unavailable]
    F --> H
    E -- Yes --> SSM[aws ssm get-parameter\n/github-runner/instance-id/ecr-cache-url]
    SSM -- Found --> G
    SSM -- Not Found --> H[🟠 ECR URL cannot be resolved\nBuild locally]
    G --> V{Validate URL format\nExtract AWS_REGION via sed}
    V -- Invalid --> ERR[🔴 Exit 1: Bad ECR URL format]
    V -- Valid --> L[ECR Login via\naws ecr get-login-password]
    L --> P{docker pull ECR image\nTag already exists?}
    P -- Yes --> EXIT0[🟢 Tag local image\nexit 0 — skip build]
    P -- No --> BUILD[docker buildx build\nwith ECR cache-from / cache-to]
    H --> BUILDLOCAL[docker buildx build\nno ECR cache]
    BUILD --> PUSH[docker push ECR image]
    PUSH --> DONE([🟢 Image ready])
    BUILDLOCAL --> DONE
    EXIT0 --> DONE
Loading

Last reviewed commit: aacdf2a

@myurasov-nv myurasov-nv merged commit dc56b0f into isaac-sim:develop Mar 10, 2026
7 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants