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

ci: Refactor cached disk image selection logic #8936

Closed
wants to merge 2 commits into from

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Oct 14, 2024

Motivation

Resolve #8908 (comment).

Solution

  • Try other locations if $PREFER_MAIN_CACHED_STATE is true and find_cached_disk_image doesn't return anything.

Tests

No tests.

PR Author's Checklist

  • The PR name is suitable for inclusion in the changelog.
  • The solution is tested.
  • The PR has a priority label.

@upbqdn upbqdn added C-bug Category: This is a bug A-devops Area: Pipelines, CI/CD and Dockerfiles C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG P-High 🔥 labels Oct 14, 2024
@upbqdn upbqdn self-assigned this Oct 14, 2024
@upbqdn upbqdn requested a review from a team as a code owner October 14, 2024 18:59
@upbqdn upbqdn requested review from oxarbitrage and removed request for a team October 14, 2024 18:59
@upbqdn upbqdn changed the title ci: Refactor disk selection logic ci: Refactor cached disk image selection logic Oct 14, 2024
Copy link
Member

@gustavovalverde gustavovalverde left a comment

Choose a reason for hiding this comment

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

Optional code improvement

fi

# If no image was found, try to find one from the current branch (or PR).
if [[ -z "${CACHED_DISK_NAME}" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

To avoid repeating the if condition several times, we can also do:

# Try to find an image based on the `main` branch if that branch is preferred.
if [[ "${PREFER_MAIN_CACHED_STATE}" == "true" ]]; then
    CACHED_DISK_NAME=$(find_cached_disk_image "main-[0-9a-f]+" "main branch")
fi
# If no image was found, try to find one from the current branch (or PR).
CACHED_DISK_NAME=${CACHED_DISK_NAME:-$(find_cached_disk_image ".+-${GITHUB_REF}" "branch")}
# If we still have no image, try to find one from any branch.
CACHED_DISK_NAME=${CACHED_DISK_NAME:-$(find_cached_disk_image ".+-[0-9a-f]+" "any branch")}

# Handle the case where no suitable disk image is found
if [[ -z "${CACHED_DISK_NAME}" ]]; then
    echo "No suitable cached state disk available. Try running the cached state rebuild job."
    exit 1
else
    echo "Selected Disk: ${CACHED_DISK_NAME}"
fi

Here we'd use ${VAR:-value} to set CACHED_DISK_NAME to the result of the next find_cached_disk_image call only if it's not already set.

gustavovalverde added a commit that referenced this pull request Oct 16, 2024
This supersedes #8936 using a different approach with `${VAR:-value}`
@gustavovalverde
Copy link
Member

Solved within #8908

mergify bot pushed a commit that referenced this pull request Oct 17, 2024
* ref(actions): allow more flexibility on cached states usage

* chore: improve message

* fix(actions): deploy single instances even if no cached state is needed

* rev: apply suggestions from code review

Co-authored-by: Marek <[email protected]>

* fix: wrong use of `failure()`

* chore: remove extra file

* chore: `echo` the pattern for easier debugging

* chore: add extra details to image naming convention

Addresses #8908 (comment)

* ref(actions): use a better logic for disk image selection

This supersedes #8936 using a different approach with `${VAR:-value}`

---------

Co-authored-by: Marek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-bug Category: This is a bug C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG P-High 🔥
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants