Skip to content

Conversation

@Nuru
Copy link
Contributor

@Nuru Nuru commented Feb 25, 2025

what

  • Fix the script for converting the current AWS IAM role to something short and meaningful for the shell command prompt
  • Add support for converting Identity Center Permission Sets to profile names in the shell prompt
  • Support setting CHAMBER_KMS_KEY_ALIAS via CHAMBER_KMS_ALIAS
  • Add the lsb-release package to provide lsb_release

why

  • The code was buggy due too piecemeal changes resulting in unreachable code
  • Many people are using Identity Center now
  • Setting CHAMBER_KMS_KEY_ALIAS as is customary in Cloud Posse Dockerfiles triggers the SecretsUsedInArgOrEnv warning. While this is arguably a bug in the warning, this change allows concerned users to avoid the issue.
  • Some tool installation scripts require lsb_release to be already installed

references

@Nuru Nuru requested a review from a team as a code owner February 25, 2025 14:02
@coderabbitai
Copy link

coderabbitai bot commented Feb 25, 2025

Warning

Rate limit exceeded

@Nuru has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 17 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 8f4e825 and d23b413.

📒 Files selected for processing (1)
  • rootfs/etc/profile.d/aws.sh (4 hunks)
📝 Walkthrough

Walkthrough

This pull request updates environment variable configurations and AWS file paths. In the Dockerfile, AWS configuration variables are removed, and a new CHAMBER variable is introduced. The aws.sh script now references AWS configuration via GEODESIC_AWS_HOME and includes refined role verification and AWS_VAULT handling. A new chamber.sh script exports CHAMBER_KMS_KEY_ALIAS based on CHAMBER_KMS_ALIAS. Additionally, the required package lsb-release is added to packages.txt, and extra comments are inserted in the ANSI colors script to clarify terminal colorization.

Changes

File(s) Change Summary
os/debian/Dockerfile.debian Removed AWS_CONFIG_FILE and AWS_SHARED_CREDENTIALS_FILE; added CHAMBER_KMS_ALIAS.
rootfs/etc/profile.d/chamber.sh Introduced chamber.sh to export CHAMBER_KMS_KEY_ALIAS based on CHAMBER_KMS_ALIAS.
rootfs/etc/profile.d/aws.sh Updated AWS credential/config paths to use ${GEODESIC_AWS_HOME}; refined role verification and enhanced AWS_VAULT handling in role-related functions.
packages.txt Appended the new package lsb-release to the packages list.
rootfs/etc/profile.d/_10-colors.sh Added explanatory comments on ANSI color codes and terminal colorization while keeping script behavior unchanged.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (10)
rootfs/etc/profile.d/chamber.sh (2)

1-1: Consider Adding a Shebang Directive.

Although profile scripts are typically sourced, adding a shebang (or a shellcheck directive) helps clarify the intended interpreter if the script is ever executed directly.

🧰 Tools
🪛 Shellcheck (0.10.0)

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)


19-22: Quote Variables in Conditional Checks.

To avoid potential word-splitting or globbing issues, please quote the variable expansions in the conditional. For example:

-if [[ -z $CHAMBER_KMS_KEY_ALIAS ]] && [[ -n $CHAMBER_KMS_ALIAS ]]; then
-	export CHAMBER_KMS_KEY_ALIAS=$CHAMBER_KMS_ALIAS
-	unset CHAMBER_KMS_ALIAS
-fi
+if [[ -z "$CHAMBER_KMS_KEY_ALIAS" ]] && [[ -n "$CHAMBER_KMS_ALIAS" ]]; then
+	export CHAMBER_KMS_KEY_ALIAS="$CHAMBER_KMS_ALIAS"
+	unset CHAMBER_KMS_ALIAS
+fi
rootfs/etc/profile.d/aws.sh (8)

118-120: Ensure Proper Quoting in AWS_VAULT Check.

Quote the expansion for added robustness. For example:

-[[ ${AWS_VAULT_ENABLED:-false} == "true" ]] || unset AWS_VAULT
+[[ "${AWS_VAULT_ENABLED:-false}" == "true" ]] || unset AWS_VAULT

126-153: Improve Robustness with Quoted Comparisons.

For all string comparisons in this block, quoting the variables helps prevent issues if their values contain spaces or unexpected characters. For example:

- if [[ $profile_arn == $current_role ]]; then
+ if [[ "$profile_arn" == "$current_role" ]]; then

Please update similar comparisons throughout this section for consistent robustness.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 126-126: Quote the right-hand side of == in [[ ]] to prevent glob matching.

(SC2053)


[warning] 138-138: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 139-139: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


158-163: Quote Variables in Mismatch Reporting.

Within the mismatch-handling section, ensure variables like AWS_PROFILE, AWS_VAULT, profile_arn, and current_role are quoted when used in conditionals and output. This will prevent unexpected behavior if any of these variables are empty or contain spaces.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 162-162: Declare and assign separately to avoid masking return values.

(SC2155)


137-139: Split Command Substitution from Declaration.

To avoid masking return values, consider splitting command substitutions from variable declarations. For example:

- local role_arn=$(printf "%s" "$current_role" | sed 's/:sts:/:iam:/g' | sed 's,:assumed-role/,:role/,')
+ local role_arn
+ role_arn=$(printf "%s" "$current_role" | sed 's/:sts:/:iam:/g' | sed 's,:assumed-role/,:role/,')
🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 138-138: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 139-139: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


137-140: Consider Using Readarray for Splitting Output.

When assigning to role_name, the command substitution splits output into an array. For improved clarity and to avoid subtle splitting issues, consider using readarray (or mapfile) to capture the output. For example:

- role_name=($(crudini --get --format=lines "$config_file" | grep "$role_arn" | cut -d' ' -f 3))
+ readarray -t role_name < <(crudini --get --format=lines "$config_file" | grep "$role_arn" | awk '{print $3}')
🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 138-138: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 139-139: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


169-170: Avoid Masking Return Values in Subshell Assignments.

Instead of assigning and exporting in one step, split the assignment to capture the output of red-n '!mixed!'. For instance:

- export ASSUME_ROLE=$(red-n '!mixed!')
+ tmp=$(red-n '!mixed!')
+ export ASSUME_ROLE="$tmp"

This approach helps ensure that any return values are not inadvertently masked.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 169-169: Declare and assign separately to avoid masking return values.

(SC2155)


180-182: Consistent Use of the Role Name Variable.

The variable role_name is sometimes used as an array (when reading multiple values via command substitution) and at other times as a simple string. For clarity and maintainability, please choose one consistent data type based on the expected number of results.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 181-181: Variable was used as an array but is now assigned a string.

(SC2178)


187-190: Be Cautious When Expanding Arrays.

If role_name is maintained as an array, ensure that any later usage correctly references its elements (e.g., using an index or proper expansion). If only one value is expected, using a string might simplify the logic.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 187-187: Expanding an array without an index only gives the first element.

(SC2128)


[warning] 188-188: Declare and assign separately to avoid masking return values.

(SC2155)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cdf5e15 and 94d0e7d.

📒 Files selected for processing (5)
  • os/debian/Dockerfile.debian (2 hunks)
  • packages.txt (1 hunks)
  • rootfs/etc/profile.d/_10-colors.sh (1 hunks)
  • rootfs/etc/profile.d/aws.sh (2 hunks)
  • rootfs/etc/profile.d/chamber.sh (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • packages.txt
  • rootfs/etc/profile.d/_10-colors.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
rootfs/etc/profile.d/chamber.sh

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

rootfs/etc/profile.d/aws.sh

[warning] 126-126: Quote the right-hand side of == in [[ ]] to prevent glob matching.

(SC2053)


[warning] 138-138: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 139-139: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


[warning] 162-162: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 169-169: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 181-181: Variable was used as an array but is now assigned a string.

(SC2178)


[warning] 187-187: Expanding an array without an index only gives the first element.

(SC2128)


[warning] 188-188: Declare and assign separately to avoid masking return values.

(SC2155)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build-and-push (debian)
🔇 Additional comments (2)
os/debian/Dockerfile.debian (1)

322-328: Workaround for SecretsUsedInArgOrEnv Warning is Clear and Concise.

The added block explains why the CHAMBER_KMS_ALIAS variable is introduced and how it will be used at runtime. Ensure that downstream scripts (like the one in chamber.sh) expect this variable.

rootfs/etc/profile.d/aws.sh (1)

124-124: Verify Role Matching Logic.

The retrieval of profile_arn via the AWS CLI (using the target profile) is key to determining if the current role matches expectations. Please double-check that the use of cut -d/ -f1-2 reliably extracts the intended segments of the ARN across various environments.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (11)
rootfs/etc/profile.d/chamber.sh (2)

1-1: Add a Shell Directive for Static Analysis
The file currently lacks a shebang or a ShellCheck directive, which triggers a static analysis warning (SC2148). If this script is intended to be executed directly, consider adding a shebang (e.g., #!/bin/bash). If it is only meant to be sourced (as is common for scripts in profile.d), you can instead add a ShellCheck comment (e.g., # shellcheck shell=bash) to clarify the expected shell and suppress the warning.

🧰 Tools
🪛 Shellcheck (0.10.0)

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)


19-22: Ensure Shell Portability in Conditional Expressions
The conditional block uses Bash-specific [[ … ]] expressions. If there is any chance that this script may be sourced in a non-Bash shell environment, consider switching to the POSIX-compliant [ … ] test syntax to enhance compatibility. For example, you might update the block as follows:

-if [[ -z "$CHAMBER_KMS_KEY_ALIAS" ]] && [[ -n "$CHAMBER_KMS_ALIAS" ]]; then
+if [ -z "$CHAMBER_KMS_KEY_ALIAS" ] && [ -n "$CHAMBER_KMS_ALIAS" ]; then

If you are certain that Bash will always be used, the current implementation is acceptable.

rootfs/etc/profile.d/aws.sh (9)

110-112: Variable Declaration with Assignment
The update (line 110–112) declares role_name and role_names and immediately assigns current_role using command substitution. Shellcheck warns (SC2155) about combining declaration with assignment, as it can mask return values. Consider separating the declaration from the assignment for improved clarity and safety. For example:

-  local current_role=$(aws sts get-caller-identity --output text --query 'Arn' 2>/dev/null | cut -d/ -f1-2)
+  local current_role
+  current_role=$(aws sts get-caller-identity --output text --query 'Arn' 2>/dev/null | cut -d/ -f1-2)
🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 112-112: Declare and assign separately to avoid masking return values.

(SC2155)


134-168: Enhanced Role Name Extraction for Default/Identity Profiles
Within the if [[ $profile_target == "default" ]] || [[ $profile_target =~ -identity$ ]] block, the logic now makes an effort to derive a more descriptive role name (e.g. for Identity Center roles) by parsing the AWS configuration file using crudini and text processing. This enhancement should help users avoid overly generic role names. Consider validating that the regular expressions and field separations (via cut) cover all expected ARN formats.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 145-145: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 148-148: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 151-151: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 154-154: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


170-182: Normal IAM Role Extraction via sed Adjustments
In the “Normal IAM role” branch, the STS ARN is transformed into an IAM role ARN using chained sed commands, and then matching profiles are gathered from the config file. While this is a clever solution, please verify that the sed expressions correctly cover all valid ARN variations and that the array assignment via command substitution is robust in your typical deployment environments.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 172-172: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 173-173: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


184-187: Fallback to Generic Profile Name
If no better match is found, the code now falls back to using the generic profile name. While this guarantees that ASSUME_ROLE is always set, it might be useful to log a warning here to indicate that a fallback was needed. This can aid in debugging potential misconfigurations later.


189-205: AWS Credentials Mismatch Handling
The mismatch-handling block explicitly warns when the current role (as determined by STS) does not match what AWS_PROFILE (or AWS_VAULT) indicates. The use of red-colored messages and setting ASSUME_ROLE to a marker (!mixed!) is clear from a user feedback perspective. However, consider whether simply marking it is sufficient or if it would be better to prompt for re-selection or even exit with an error under certain conditions.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 196-196: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 203-203: Declare and assign separately to avoid masking return values.

(SC2155)


213-217: Role Name Extraction from Credentials File
The update retrieves a potential role name from the shared credentials file using crudini and text processing. This fallback mechanism is valuable. Just be mindful that if the file’s format changes or contains extra entries, the use of head -1 may not always pick the ideal match.


220-224: Fallback Role Name Retrieval from AWS Config File
If the credentials file does not yield a role name, the code then checks the AWS config file. The transformation of the current_role into an IAM role ARN (via sed) and the subsequent extraction using crudini is consistent with earlier logic. Please ensure that these text manipulations reliably handle all cases, especially if AWS updates its ARN formats in the future.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 222-222: Declare and assign separately to avoid masking return values.

(SC2155)


226-244: Generating a Fallback Role Name When None is Found
When no profile name is determined from the various lookups, the script now generates a fallback name. In the branch handling Identity Center roles, account ID and SSO role name are extracted from the ARN. While this fallback is a practical solution, consider whether it might be beneficial to log this occurrence with a warning level to alert users that an expected profile name was not found.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 234-234: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 236-236: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 238-238: Declare and assign separately to avoid masking return values.

(SC2155)


112-112: Addressing Shellcheck Warnings (SC2155 & SC2207)
Multiple command substitutions paired with variable declarations (e.g. at lines 112, 125, 145, etc.) have been flagged by Shellcheck. Although these may not impact functionality immediately, refactoring such lines to separate the declaration from the assignment can prevent potential masking of return values and improve readability.
For instance, consider changing:

-  local current_role=$(aws sts get-caller-identity --output text --query 'Arn' 2>/dev/null | cut -d/ -f1-2)
+  local current_role
+  current_role=$(aws sts get-caller-identity --output text --query 'Arn' 2>/dev/null | cut -d/ -f1-2)

Please review similar patterns elsewhere in the script.

Also applies to: 124-124, 145-145, 148-148, 151-151, 154-154, 172-172, 173-173, 196-196, 203-203, 222-222, 234-234, 236-236, 238-238

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 112-112: Declare and assign separately to avoid masking return values.

(SC2155)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94d0e7d and 60b6b6f.

📒 Files selected for processing (2)
  • rootfs/etc/profile.d/aws.sh (2 hunks)
  • rootfs/etc/profile.d/chamber.sh (1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
rootfs/etc/profile.d/aws.sh

[warning] 112-112: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 145-145: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 148-148: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 151-151: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 154-154: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


[warning] 172-172: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 173-173: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


[warning] 196-196: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 203-203: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 222-222: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 234-234: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 236-236: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 238-238: Declare and assign separately to avoid masking return values.

(SC2155)

rootfs/etc/profile.d/chamber.sh

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build-and-push (debian)
🔇 Additional comments (4)
rootfs/etc/profile.d/aws.sh (4)

64-77: Consistent AWS Credentials & Config File Fallbacks
The modified aws_choose_role now uses fallbacks based on ${GEODESIC_AWS_HOME} (i.e. ${GEODESIC_AWS_HOME}/credentials and ${GEODESIC_AWS_HOME}/config) to source AWS configuration. This approach improves consistency with the overall script design.


118-120: Clearing AWS_VAULT When Not Enabled
The concise use of the conditional expression
  [[ "${AWS_VAULT_ENABLED:-false}" == "true" ]] || unset AWS_VAULT
effectively ensures that AWS_VAULT is cleared when not enabled. This logic is both succinct and clear; please double-check downstream dependencies to be sure that unsetting AWS_VAULT does not cause any side effects.


124-127: Retrieving Profile ARN for Role Comparison
The new code block retrieves the profile’s ARN via the AWS CLI using the determined profile_target and then extracts the base ARN portion with cut. This is a key step to later compare it with the current role ARN. Make sure the command handles errors gracefully (e.g. in cases where the AWS CLI might not return the expected output).


207-210: Clarification of the API Keys Scenario
This comment block clarifies that when neither AWS_VAULT nor AWS_PROFILE is in use, the script will try alternative methods to determine a role name, likely because API keys are directly set. The descriptive comments help in understanding the control flow—no changes required but clarity is appreciated.

@Nuru
Copy link
Contributor Author

Nuru commented Feb 25, 2025

@coderabbitai how do I suppress SC2155

@coderabbitai
Copy link

coderabbitai bot commented Feb 25, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@Nuru The ShellCheck warning SC2155 occurs when you declare and assign a variable in the same statement (e.g., export VAR=$(command)). This masks the return status of the command, preventing error detection.

You can suppress it in several ways:

  1. Add an inline comment before the line:

    # shellcheck disable=SC2155
    export VAR=$(command)
  2. Disable at the beginning of the file (affects the entire file):

    # shellcheck disable=SC2155
  3. Create a .shellcheckrc file in your repository root with:

    disable=SC2155
    
  4. The proper fix (preferred) is to split declaration and assignment:

    VAR=$(command)
    export VAR

Would you like me to help you implement any of these solutions in specific files?


🏁 Script executed:

#!/bin/bash
# Verify the ShellCheck warning SC2155 by searching for it in the repository
rg "SC2155" -A 5 -B 5

# Let's also check if there are any ShellCheck configuration files 
fd .shellcheckrc
fd shellcheck.config
fd shellcheck.json

# Let's also check if there are any shell scripts that might be triggering SC2155 warnings
fd -t f -e sh | xargs grep -l "export.*=" | head -5

# Let's try to understand more about the context in which ShellCheck is being used
rg "shellcheck" --include="*.sh" --include="*.bash" --include="Makefile" --include="*.yaml" --include="*.yml"

Length of output: 866


🌐 Web query:

What is ShellCheck warning SC2155 and how to suppress it?

💡 Result:

ShellCheck warning SC2155 occurs when a variable is declared (using local, export, or readonly) and assigned a value via command substitution in a single line. This masks the return value of the substituted command, potentially hiding errors from commands like $(...) [8].

Key Issues Addressed by SC2155:

  1. Exit Code Masking:
    When combining declaration with assignment (e.g., local var="$(cmd)"), only the declaration’s success/failure is checked—not the substituted command’s exit code. This can bypass error handling mechanisms like set -e [4][8].

  2. Portability Risks:
    Some shells (e.g., older versions of Dash) fail to handle quoted assignments with spaces in declarations like local var="$(printf 'a b')", leading to syntax errors [8].

How to Fix:

Refactor declarations and assignments into separate steps:

# Before (triggers SC2155)
readonly SCRIPT_FILE="$(readlink -f "$0")"

# After (no warning)
SCRIPT_FILE="$(readlink -f "$0")"
readonly SCRIPT_FILE

For variables like export or local:

# Before
export PATH="$(custom_path)"

# After
PATH="$(custom_path)"
export PATH

Suppressing SC2155:

  1. Inline Directive:
    Disable the warning for specific lines:

    # shellcheck disable=SC2155
    readonly TIMESTAMP="$(date +%s)"
  2. Command-Line Flag:
    Exclude SC2155 during ShellCheck execution:

    shellcheck -e SC2155 script.sh
  3. Configuration File:
    Add to .shellcheckrc:

    disable=SC2155

Exceptions:

  • No warning is generated for literals (e.g., export FOO=bar) since no command substitution occurs [8].
  • For readonly, some shells require splitting declaration and assignment due to syntax limitations [9].

Citations:


@Nuru To suppress ShellCheck warning SC2155 (which warns about declaring and assigning a variable in a single statement), you have several options:

  1. Inline suppression - Add a comment before the problematic line:

    # shellcheck disable=SC2155
    export VAR=$(command)
  2. Block suppression - Disable for multiple lines:

    # shellcheck disable=SC2155
    export VAR1=$(command1)
    export VAR2=$(command2)
    # shellcheck enable=SC2155
  3. File-level suppression - Add to the top of your script:

    # shellcheck shell=bash
    # shellcheck disable=SC2155
  4. Repository-level suppression - Create a .shellcheckrc file in the repository root:

    disable=SC2155
    
  5. Fix the warning (recommended approach) - Split declaration and assignment:

    VAR=$(command)
    export VAR

Would you like me to help implement any of these solutions in specific files in your repository?


🏁 Script executed:

#!/bin/bash
# Let's try to find shell scripts with potential SC2155 issues
fd -t f -e sh | xargs grep -l "export.*\$(" | head -5

# Check for any existing shellcheck directives
rg "shellcheck disable" --type sh

# Try a better shellcheck search command
grep -r "shellcheck" --include="*.sh" --include="*.bash" --include="Makefile" .

Length of output: 562


@Nuru I can see your repository already uses shellcheck directives in some files (like _10-colors.sh). Based on the code style in your repository, here's how to suppress SC2155 warnings:

  1. Add an inline directive for specific lines (matches your existing style):

    # shellcheck disable=SC2155
    export AWS_PROFILE=$(aws_choose_role "${PROFILE}")
  2. For multiple issues in the same file (like in aws.sh), you can add it at the top:

    #!/bin/bash
    # shellcheck disable=SC2155
  3. The best practice fix is to split the declaration and export:

    # Instead of:
    export AWS_PROFILE=$(aws_choose_role "${PROFILE}")
    
    # Use:
    AWS_PROFILE=$(aws_choose_role "${PROFILE}")
    export AWS_PROFILE

Would you like me to locate specific instances of SC2155 in your files and suggest fixes for them?

@coderabbitai
Copy link

coderabbitai bot commented Feb 25, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@Nuru There are several ways to suppress ShellCheck warning SC2155 ("Declare and assign separately to avoid masking return values"):

  1. Inline suppression (for specific instances):
    bash
    VAR=$(command)
    export VAR

This way, you can check the return status of the command before exporting.

---

🏁 Script executed:

#!/bin/bash

Check if there's a ShellCheck configuration file in the repository

echo "Checking for ShellCheck configuration files:"
fd -t f .shellcheckrc
fd -t f .shellcheckrc.toml
fd -t f shellcheck.toml

Check if there are any specific shellcheck directives already in the codebase

echo -e "\nChecking for existing ShellCheck directives:"
rg "shellcheck disable=|# shellcheck disable=|#shellcheck disable=" --type=sh

Look for examples that might trigger SC2155 in shell scripts

echo -e "\nChecking for potential SC2155 triggers in shell scripts:"
rg "export $(" --type=sh
rg "export [A-Za-z0-9_]+=$(" --type=sh
rg "local [A-Za-z0-9_]+=$(" --type=sh

Show the SC2155 warning description directly from ShellCheck if available

echo -e "\nAttempting to get SC2155 description from ShellCheck:"
shellcheck --help | grep SC2155 || echo "ShellCheck command not available"


Length of output: 964

---

🏁 Script executed:

shellcheck disable=SC2155

export VAR=$(command)


2. **Block suppression** (for a section of code):
```bash
# shellcheck disable=SC2155
{
  export VAR1=$(command1)
  export VAR2=$(command2)
}
  1. File-level suppression (at the top of the file):

    #!/bin/bash
    # shellcheck disable=SC2155
  2. Global suppression (create a .shellcheckrc file in the repository root):

    disable=SC2155
    

The proper fix (rather than suppression) is to declare and assign separately:


Length of output: 1658

---

@Nuru To suppress ShellCheck warning SC2155 (which warns about "Declare and assign separately to avoid masking return values"), you have several options:

1. **Inline suppression** (best for isolated cases):
   bash
VAR=$(command)
export VAR

This second approach is generally preferred as it allows you to check the return status of the command before exporting.


🏁 Script executed:

#!/bin/bash
# Look at the modified shell scripts in the PR
echo "Examining shell scripts in PR:"
git show --name-only HEAD | grep ".sh$" || echo "No shell scripts found in PR"

# Let's look at the modified shell scripts to find potential SC2155 issues
echo -e "\nLooking at content of shell scripts in the PR:"
for file in $(git show --name-only HEAD | grep ".sh$"); do
  if [ -f "$file" ]; then
    echo -e "\n=== $file ===\n"
    cat "$file"
  fi
done

# Find examples of variable assignments in shell scripts that might trigger SC2155
echo -e "\nSearching for potential SC2155 triggers in the modified files:"
for file in $(git show --name-only HEAD | grep ".sh$"); do
  if [ -f "$file" ]; then
    echo -e "\n=== $file ===\n"
    grep -n "export.*=\$(.*)" "$file" || echo "No direct export with command substitution found"
  fi
done

# Look specifically at the aws.sh file which was modified in this PR
echo -e "\nLooking for SC2155 triggers in aws.sh:"
if [ -f "rootfs/etc/profile.d/aws.sh" ]; then
  grep -n "export.*=\$(.*)" rootfs/etc/profile.d/aws.sh || echo "No direct export with command substitution found"
fi

# Let's check all existing suppress directives in the codebase
echo -e "\nCheck existing ShellCheck suppress examples in the codebase:"
rg -n "# shellcheck disable=" --type=sh | head -10

Length of output: 69785


🏁 Script executed:

   # shellcheck disable=SC2155
   export VAR=$(command)
  1. File-level suppression (add at the top of your script):

    #!/bin/bash
    # shellcheck disable=SC2155
  2. Create a .shellcheckrc file in the repository root with:

    disable=SC2155
    

Looking at your codebase, I see you already use shellcheck directives in some files like _10-colors.sh, so the inline approach is consistent with your existing practices.

Alternatively, you could fix the warning by declaring and assigning separately:


Length of output: 836

---

@Nuru To suppress ShellCheck warning SC2155 (which warns about declaring and assigning a variable in the same statement), you have three options:

1. **Inline suppression** (preferred for isolated cases):
   ```bash
   # shellcheck disable=SC2155
   export SOME_VAR=$(command)
  1. File-level suppression (for multiple instances in one file):

    #!/bin/bash
    # shellcheck disable=SC2155
    
    # All export/declare+assign statements in this file will be ignored
  2. Global suppression (create a .shellcheckrc file in repository root):

    disable=SC2155
    

Based on your existing code style, the inline method is most consistent with how you're already using ShellCheck directives in other files like _10-colors.sh.


🏁 Script executed:

#!/bin/bash
# Let's find some examples of code that would trigger SC2155 warnings in the codebase
echo "Looking for export statements with command substitution (common SC2155 triggers):"
rg "export [A-Za-z0-9_]+=\$\(" --type=sh -A 1 -B 1

echo -e "\nLooking for local variable declarations with command substitution:"
rg "local [A-Za-z0-9_]+=\$\(" --type=sh -A 1 -B 1

# Let's look at the file the user is modifying in the PR
echo -e "\nInspecting the specific aws.sh file modified in PR:"
cat rootfs/etc/profile.d/aws.sh

echo -e "\nChecker for instances where the SC2155 warning is already suppressed:"
rg "# shellcheck disable=SC2155" --type=sh

Length of output: 12092


@Nuru To suppress ShellCheck warning SC2155 (declaring and assigning a variable in the same statement), you have three options:

  1. Inline suppression (for specific lines):

    # shellcheck disable=SC2155
    export VAR=$(command)
  2. File-level suppression (add at the top of your script):

    #!/bin/bash
    # shellcheck disable=SC2155
  3. Global suppression (create a .shellcheckrc file in repository root):

    disable=SC2155
    

Looking at your aws.sh file, you have several instances that would trigger this warning, particularly with command substitution inside exports and local variable declarations.

If you prefer to fix the warnings instead of suppressing them, separate the declaration from the assignment:

# Instead of
export VAR=$(command)

# Do this
VAR=$(command)
export VAR

The inline method is generally recommended for isolated cases, while file-level suppression works well if there are many instances in a single file.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
rootfs/etc/profile.d/aws.sh (2)

137-167: Complex Role Name Extraction Logic.
The nested conditional block for extracting a more descriptive role name—especially for Identity Center roles—is comprehensive. However, its complexity could affect maintainability. Consider refactoring this logic into a dedicated helper function to improve clarity in future maintenance.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 157-157: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


258-258: Quote Parameter Expansion in the stat Command.
The stat command uses a parameter expansion for determining the modification time of the credentials file. To avoid any word-splitting issues (especially if the path contains spaces), enclose the expansion in quotes as shown below:

- local credentials_mtime=$(stat -c "%Y" ${AWS_SHARED_CREDENTIALS_FILE:-"${GEODESIC_AWS_HOME}/credentials"} 2>/dev/null)
+ local credentials_mtime=$(stat -c "%Y" "${AWS_SHARED_CREDENTIALS_FILE:-${GEODESIC_AWS_HOME}/credentials}" 2>/dev/null)

This ensures that the file path is properly interpreted.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 60b6b6f and 8f4e825.

📒 Files selected for processing (1)
  • rootfs/etc/profile.d/aws.sh (4 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
rootfs/etc/profile.d/aws.sh

[warning] 157-157: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


[warning] 176-176: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build-and-push (debian)
🔇 Additional comments (7)
rootfs/etc/profile.d/aws.sh (7)

2-4: Clarify ShellCheck Directive.
The inline directive and accompanying comment clearly explain why SC2155 is suppressed. This is acceptable in this context given that the script intentionally does not check the return values here.


68-68: Update AWS Credential and Config File Defaults.
The cat command now uses default paths based on ${GEODESIC_AWS_HOME} for both credentials and config files. This change aligns with the PR objective of consolidating AWS configuration paths. Please verify that all parts of the script consistently reference these new defaults.


113-113: Use of Local Variable Declarations.
Declaring role_name and role_names as local variables is a good practice that limits their scope.


121-123: Effective Handling of AWS_VAULT Configuration.
Clearing AWS_VAULT when it isn’t enabled is handled concisely with the inline conditional. This ensures that extraneous settings do not persist in the environment.


128-128: Profile ARN Retrieval.
The use of aws --profile "${profile_target}" ... to fetch the role ARN is clear and the redirection of errors helps keep the output clean. Verify that this silent failure behavior is intended in all scenarios.


130-130: Role Matching Check.
The check comparing "$profile_arn" to "$current_role" is direct and effectively ensures that the active ARN matches the expected profile.


1-271: Overall Consistency in AWS Configuration Handling.
The modifications made throughout the script ensure that AWS configuration and credential file paths now consistently reference ${GEODESIC_AWS_HOME}. This unified approach reduces ambiguity and potential conflicts that could arise from defaulting to disparate paths. Excellent work aligning these changes with the PR objectives.

🧰 Tools
🪛 Shellcheck (0.10.0)

[warning] 105-105: Use "$@" (with quotes) to prevent whitespace problems.

(SC2048)


[warning] 157-157: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


[warning] 176-176: Prefer mapfile or read -a to split command output (or quote to avoid splitting).

(SC2207)


[warning] 260-260: Quote the right-hand side of != in [[ ]] to prevent glob matching.

(SC2053)

@Nuru Nuru merged commit 4f2eac5 into main Feb 25, 2025
4 checks passed
@Nuru Nuru deleted the role-detect branch February 25, 2025 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants