Skip to content

Conversation

@dgchinner
Copy link
Contributor

@dgchinner dgchinner commented Feb 11, 2026

This results in a failure due to the '-eu' flags the shell is running under like this:

systemd[1]: Starting Customisations based on SKU...
setup_sku_customisations.sh[1343]: /opt/hpc/azure/bin/setup_sku_customisations.sh: line 29: __MOCK_SKU: unbound variable
systemd[1]: sku_customisation.service: Main process exited, code=exited, status=1/FAILURE

The test code can validly set __MOCK_SKU to an
empty string to test fetching the SKU from the azure infrastructure. Hence we need to have both a defined, empty string and an undefined environment variable work correct. This is how the "-z $__MOCK_SKU" check is defined to work, but we have to work around linter requirements for avoiding use of undefined variables.

Hence using 'test -v __MOCK_SKU' to detect if the variable is undefined is insufficient to trigger auto-test behaviour as it does not capture the defined-but-empty case described above. As such, use the bash conditional variable init construct that initialises __MOCK_SKU to an empty string if and only if the variable is not already defined by the environment.

Also clean up a couple of other minor issues that were also introduced when satisfying linters found whilst testing the above fix.

Issue Tracker Tickets (Jira or BZ if any): RHELHPC-152

Summary by Sourcery

Ensure SKU customisation script handles undefined or empty __MOCK_SKU correctly while keeping tests compatible with strict shell settings.

Bug Fixes:

  • Prevent setup_sku_customisations.sh from failing under -u due to __MOCK_SKU being unset when run from init services.
  • Fix test harness invocation so mocked SKU value is passed separately from the script path.

Enhancements:

  • Create the topology runtime directory before writing topology artifacts to it.
  • Adjust test-sku-setup.sh shell options to better reflect expected failures in test code while still catching use of unset variables.

@sourcery-ai
Copy link

sourcery-ai bot commented Feb 11, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adjusts SKU customisation scripts to safely handle an optionally undefined __MOCK_SKU under strict shell settings, ensures topology runtime directory exists, and clarifies test behaviour in the SKU test harness.

Sequence diagram for __MOCK_SKU initialisation and SKU lookup

sequenceDiagram
  actor Systemd
  participant Script as setup_sku_customisations_sh
  participant Env as Environment
  participant Metadata as AzureMetadataService

  Systemd ->> Script: Start sku_customisation.service
  Script ->> Script: Set topology env vars
  Script ->> Script: mkdir -p TOPOLOGY_RUNTIME_DIR
  Script ->> Env: Read __MOCK_SKU
  alt __MOCK_SKU undefined in Env
    Script ->> Script: Parameter expansion ${__MOCK_SKU:=}
    Script ->> Env: Set __MOCK_SKU to empty string
  else __MOCK_SKU defined (possibly empty)
    Script ->> Script: Keep existing __MOCK_SKU
  end
  Script ->> Script: Test -z "$__MOCK_SKU"
  alt __MOCK_SKU empty
    Script ->> Metadata: HTTP GET /metadata/instance
    Metadata -->> Script: SKU from metadata
    Script ->> Script: Apply SKU customisations
  else __MOCK_SKU nonempty (test harness)
    Script ->> Script: Use mocked SKU from Env
    Script ->> Script: Apply SKU customisations
  end
  Script -->> Systemd: Exit status 0 (on success)
Loading

Flow diagram for SKU resolution logic in setup_sku_customisations.sh

flowchart TD
  A_start[Start setup_sku_customisations.sh] --> B_set_dirs[Set topology environment variables]
  B_set_dirs --> C_mkdir[Create TOPOLOGY_RUNTIME_DIR with mkdir -p]
  C_mkdir --> D_init_mock_sku[Initialise __MOCK_SKU with parameter expansion]
  D_init_mock_sku --> E_check_mock_sku{Is __MOCK_SKU empty?}
  E_check_mock_sku -->|Yes| F_use_metadata[Call Azure metadata endpoint to fetch SKU]
  E_check_mock_sku -->|No| G_use_mock_sku[Use __MOCK_SKU from environment or tests]
  F_use_metadata --> H_apply_customisations[Apply SKU specific customisations]
  G_use_mock_sku --> H_apply_customisations
  H_apply_customisations --> I_end[End script]
Loading

File-Level Changes

Change Details Files
Ensure __MOCK_SKU is always defined (possibly as empty) so script works under -u while preserving test semantics for unset vs empty.
  • Add a bash parameter expansion to initialise __MOCK_SKU to an empty string only when the variable is currently undefined in the environment
  • Retain the existing logic that treats an empty __MOCK_SKU as the signal to fetch the SKU from the Azure metadata service
templates/sku/setup_sku_customisations.sh
Adjust test SKU harness shell options and invocation so failures are allowed while still treating unset variables as errors, and fix how __MOCK_SKU is passed into the script under test.
  • Change the shebang to drop -eu and instead explicitly enable set -u so that non-zero exits are allowed but unbound variables still fail
  • Fix the manual test path to set __MOCK_SKU to just the SKU and invoke setup_sku_customisations.sh as a separate argument so the variable is not polluted with the script path
templates/sku/test-sku-setup.sh
Ensure topology runtime directory exists before using it.
  • Create the topology runtime directory before writing graph/topology files into it to avoid failures when the directory is missing
templates/sku/setup_sku_customisations.sh

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • In test-sku-setup.sh, the change from __MOCK_SKU="$sku {{ __hpc_azure_resource_dir }}/bin/setup_sku_customisations.sh" to __MOCK_SKU="$sku" "{{ __hpc_azure_resource_dir }}/bin/setup_sku_customisations.sh" alters the command invocation semantics; if the intent is to set __MOCK_SKU in the environment while running setup_sku_customisations.sh, consider using __MOCK_SKU="$sku" "{{ __hpc_azure_resource_dir }}/bin/setup_sku_customisations.sh" (single command line) or __MOCK_SKU="$sku" bash "{{ __hpc_azure_resource_dir }}/bin/setup_sku_customisations.sh" to preserve the original behavior more explicitly.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `test-sku-setup.sh`, the change from `__MOCK_SKU="$sku {{ __hpc_azure_resource_dir }}/bin/setup_sku_customisations.sh"` to `__MOCK_SKU="$sku" "{{ __hpc_azure_resource_dir }}/bin/setup_sku_customisations.sh"` alters the command invocation semantics; if the intent is to set `__MOCK_SKU` in the environment while running `setup_sku_customisations.sh`, consider using `__MOCK_SKU="$sku" "{{ __hpc_azure_resource_dir }}/bin/setup_sku_customisations.sh"` (single command line) or `__MOCK_SKU="$sku" bash "{{ __hpc_azure_resource_dir }}/bin/setup_sku_customisations.sh"` to preserve the original behavior more explicitly.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

This results in a failure due to the '-eu' flags the shell is
running under like this:

 systemd[1]: Starting Customisations based on SKU...
 setup_sku_customisations.sh[1343]: /opt/hpc/azure/bin/setup_sku_customisations.sh: line 29: __MOCK_SKU: unbound variable
 systemd[1]: sku_customisation.service: Main process exited, code=exited, status=1/FAILURE

The test code can validly set __MOCK_SKU to an
empty string to test fetching the SKU from the azure infrastructure.
Hence we need to have both a defined, empty string and an undefined
environment variable work correct. This is how the "-z $__MOCK_SKU"
check is defined to work, but we have to work around linter
requirements for avoiding use of undefined variables.

Hence using 'test -v __MOCK_SKU' to detect if the variable is
undefined is insufficient to trigger auto-test behaviour as it does
not capture the defined-but-empty case described above. As such,
use the bash conditional variable init construct that initialises
__MOCK_SKU to an empty string if and only if the variable is not
already defined by the environment.

Also clean up a couple of other minor issues that were also
introduced when satisfying linters found whilst testing the above
fix.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants