Enhance power operation handling and validation#144
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances ztictl for production environments with comprehensive CI/CD support. The changes focus on making the tool more robust in automated and enterprise environments by adding non-interactive modes, production-grade HTTP clients, concurrency safety, and authentication detection.
Key Changes:
- Production-ready version checking with proxy support, retry logic, and GitHub token authentication
- CI/CD integration with automatic non-interactive mode detection and IAM-based authentication
- Concurrency safety improvements with mutex protection for shared manager state
- Comprehensive CI/CD documentation and workflow examples
Reviewed Changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ztictl/pkg/version/checker.go | Enhanced HTTP client with proxy support, TLS configuration, retry logic, and GitHub authentication for production environments |
| ztictl/internal/ssm/manager.go | Added mutex protection for concurrent initialization of platform components and managers |
| ztictl/internal/ssm/manager_test.go | Added concurrency tests to verify thread-safe initialization |
| ztictl/internal/splash/splash.go | Added terminal detection and CI environment detection to suppress splash screen in non-interactive contexts |
| ztictl/internal/splash/splash_test.go | Updated tests to verify splash screen suppression in non-terminal environments |
| ztictl/internal/auth/sso.go | Added environment credential detection functions for CI/CD authentication validation |
| ztictl/cmd/ztictl/root.go | Added execution context, CI detection, non-interactive mode support, and environment-based configuration |
| ztictl/cmd/ztictl/config.go | Enhanced config initialization to support non-interactive mode with environment variables |
| ztictl/cmd/ztictl/auth.go | Added CI/CD detection and helpful error messages when SSO auth is attempted in automated environments |
| ztictl/cmd/ztictl/ssm_power.go | Added state validation before power operations and improved parallel execution |
| ztictl/cmd/ztictl/ssm_power_test.go | Added comprehensive tests for state validation and power operation error handling |
| ztictl/go.mod, ztictl/go.sum | Updated AWS SDK and dependency versions |
| docs/CI_CD_AUTHENTICATION.md | Comprehensive guide for CI/CD authentication methods and platform-specific examples |
| docs/VERSION_CHECKING.md | Documentation of version checking architecture and environment variables |
| docs/examples/*.{yml,groovy} | Complete CI/CD workflow examples for GitHub Actions, GitLab CI, and Jenkins |
| ztictl/README.md, README.md | Added CI/CD authentication documentation and references |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ztictl/cmd/ztictl/config.go (1)
182-188: Non-interactiveconfig init --forcestill launches the interactive wizard
Lines 182-188 callrunInteractiveConfigwheneverforceis true. In CI/non-interactive mode we pass--force(or rely onexecCtx.AutoYes) precisely to allow overwriting without prompts, but this branch still drops into the wizard and blocks the pipeline. Please gate the interactive path on theinteractiveflag only and let the non-interactive branch (Line 199 onward) handle the overwrite case.- // Run interactive setup if force is now true (either from flag or user confirmation) - if interactive || force { + // Run interactive setup only when explicitly requested + if interactive { if err := runInteractiveConfig(configPath); err != nil { return fmt.Errorf("interactive configuration failed: %w", err) } return nil }
🧹 Nitpick comments (5)
scripts/send-release-notification.sh (2)
94-103: Variable declaration style is valid but consider consistency.The refactoring to separate
localdeclarations from assignments is syntactically correct and can improve clarity in some cases. However, the more common Bash idiom islocal var=$(command)on a single line unless there's a specific reason to separate them.Consider whether this style should be standardized across all notification scripts for consistency.
86-87: Remove unnecessary SC2317 suppression or investigate shellcheck resolution.The function
validate_release_paramsis properly defined insrc/00_utils.sh(line 317) and the utilities file is correctly sourced (line 22). However, SC2317 (unreachable code) has no target at lines 86-87—the function call is the last statement in its block with no subsequent code. The suppression likely masks a false positive where shellcheck cannot resolve the sourced function. Either remove the suppression and verify shellcheck passes, or investigate the shellcheck configuration to ensure it properly resolves sourced functions.scripts/send-pr-notification.sh (1)
116-132: Variable declaration refactoring matches release notification script.This refactoring follows the same pattern as
send-release-notification.sh. The separate declaration and assignment style is valid, though consistency across the codebase should be considered..github/workflows/build.yml (1)
70-74: Include new scripts in the syntax validation loop.You added
01_install.sh/02_uninstall.shto the shellcheck run, but thebash -nloop right below still skips them. Please add the two filenames to that loop so the syntax check continues to cover the renamed scripts.Apply this diff (or equivalent):
- for script in authaws ssm src/*.sh tools/*.sh; do + for script in authaws ssm src/*.sh tools/*.sh 01_install.sh 02_uninstall.sh; dodocs/examples/github-actions-oidc.yml (1)
58-60: Recommend bumping checkout action to v4
Line 59 still referencesactions/checkout@v3, which runs on the deprecated Node 16 runtime. GitHub runners are migrating to Node 20, so keeping v3 in the example risks breakage for users who copy it verbatim. Switching the example toactions/checkout@v4keeps it aligned with the supported runtime and avoids future failures.- - name: Checkout code - uses: actions/checkout@v3 + - name: Checkout code + uses: actions/checkout@v4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ztictl/go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
.github/workflows/build.yml(3 hunks)INSTALLATION.md(2 hunks)README.md(2 hunks)docs/CI_CD_AUTHENTICATION.md(1 hunks)docs/CI_CD_PIPELINE.md(3 hunks)docs/PRODUCTION_VERSION_FIX.md(1 hunks)docs/QUICK_START.md(1 hunks)docs/VERSION_CHECKING.md(1 hunks)docs/examples/github-actions-oidc.yml(1 hunks)docs/examples/gitlab-ci-oidc.yml(1 hunks)docs/examples/jenkins-iam-keys.groovy(1 hunks)scripts/build-windows-dev.sh(3 hunks)scripts/send-pr-notification.sh(2 hunks)scripts/send-release-notification.sh(2 hunks)ztictl/README.md(2 hunks)ztictl/cmd/ztictl/auth.go(1 hunks)ztictl/cmd/ztictl/config.go(4 hunks)ztictl/cmd/ztictl/config_test.go(1 hunks)ztictl/cmd/ztictl/root.go(7 hunks)ztictl/cmd/ztictl/ssm_power.go(9 hunks)ztictl/cmd/ztictl/ssm_power_test.go(3 hunks)ztictl/go.mod(2 hunks)ztictl/internal/auth/sso.go(2 hunks)ztictl/internal/splash/splash.go(8 hunks)ztictl/internal/splash/splash_test.go(1 hunks)ztictl/internal/ssm/manager.go(5 hunks)ztictl/internal/ssm/manager_test.go(1 hunks)ztictl/pkg/version/checker.go(4 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/CI_CD_AUTHENTICATION.md
[style] ~85-~85: The double modal “required Stored” is nonstandard (only accepted in certain dialects). Consider “to be Stored”.
Context: ...rity risk) - Manual rotation required - Stored in secrets manager (additional exposure...
(NEEDS_FIXED)
docs/PRODUCTION_VERSION_FIX.md
[uncategorized] ~5-~5: The official name of this software platform is spelled with a capital “H”.
Context: ... support (corporate firewalls block api.github.com) - No TLS certificate handling (cor...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
docs/VERSION_CHECKING.md
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
14-14: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
19-19: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
24-24: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
30-30: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/CI_CD_AUTHENTICATION.md
241-241: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
265-265: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
792-792: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
815-815: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
876-876: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (28)
ztictl/internal/splash/splash_test.go (1)
31-60: LGTM! Test correctly validates non-terminal behavior.The test expectations have been properly updated to reflect that splash screens should not be shown in non-terminal environments (like test runners). The comments clearly document this behavior.
ztictl/internal/ssm/manager.go (4)
42-42: Thread-safety enhancement with mutex field.The addition of the mutex field is part of the broader thread-safety improvements to the Manager struct.
144-149: LGTM! Proper mutex protection for initialization.The mutex lock with deferred unlock and early return pattern correctly prevents concurrent initialization race conditions. The check-after-lock pattern ensures idempotency.
177-201: LGTM! Thread-safe manager initialization with proper guards.The implementation correctly:
- Acquires the mutex before checking initialization state
- Uses nil checks to prevent redundant initialization
- Initializes managers only when needed
- Maintains thread safety with deferred unlock
583-583: Minor optimization: pre-allocated slice capacity.Pre-allocating the slice with the source length is a good micro-optimization that can reduce allocations during the append operations.
scripts/build-windows-dev.sh (4)
17-17: LGTM! Improved error handling with explicit exit on cd failure.The addition of
|| exitaftercdcommands ensures the script terminates immediately if directory changes fail, preventing commands from executing in the wrong directory.Also applies to: 27-27
31-56: LGTM! Conditional build structure improves error handling.The refactoring to wrap the build command in an
ifstatement with explicit success/failure branches is a significant improvement. The script now properly handles build failures and provides clear feedback.
49-49: Correct escaping for Windows paths in printf.The change from
echotoprintfwith\\\\correctly produces a single backslash in the output, which is necessary for the Windows PowerShell command. This is the proper way to handle path separators in cross-platform scripts.
59-60: SC2119 suppression is necessary and appropriate.The
log_completion()function insrc/00_utils.shreferences the$1parameter with a default value (${1:-}), and at line 60 it's called without arguments. ShellCheck correctly identifies this as SC2119 (function references parameters but called without arguments). Since the function is intentionally designed to work without arguments using a fallback mechanism, the suppression is justified.docs/examples/jenkins-iam-keys.groovy (5)
1-23: Excellent documentation and security guidance.The header clearly explains prerequisites, security considerations, and trade-offs between IAM keys and instance profiles. The warning about key rotation is particularly valuable.
27-32: LGTM! Proper environment configuration for non-interactive execution.The environment variables correctly configure ztictl for CI/CD usage:
ZTICTL_NON_INTERACTIVE='true'prevents interactive promptsZTICTL_LOG_ENABLED='false'keeps CI logs clean- Region settings provide sensible defaults
59-75: Proper credential management with validation steps.The use of
withCredentialsensures sensitive data isn't logged, and the validation steps (aws sts get-caller-identity,ztictl config check) provide early failure detection.
160-180: Excellent failure handling with artifact collection.The failure block downloads logs for debugging and uses
|| trueto prevent the log download from failing the post-action. TheallowEmptyArchiveparameter is also a good defensive practice.
184-365: Comprehensive alternative examples cover common use cases.The commented examples demonstrate:
- EC2 instance profile authentication (more secure)
- Multi-region deployments with parameterization
- Test environment lifecycle management
These provide valuable guidance for different Jenkins deployment scenarios.
ztictl/go.mod (1)
8-16: I'll verify that these AWS SDK versions are valid and check for any known compatibility issues.No security vulnerabilities detected; versions are valid and current.
The AWS SDK for Go v2 version v1.39.6 is a legitimate release published on Nov 4, 2025. The GitHub security advisory check returned no vulnerabilities for the aws-sdk-go-v2 package. The individual AWS service modules (ec2, iam, s3, ssm, sso, ssooidc, sts) maintain independent version numbering and release cycles, which explains the variation in version numbers across the go.mod entries.
scripts/send-pr-notification.sh (1)
108-109: Remove the unnecessary SC2317 suppression.The
validate_notification_paramsfunction is properly defined in the sourced utilities file and called in a reachable code path. The suppression is not needed—there is no unreachable code here. The function is defined at line 284 ofsrc/00_utils.sh, the utilities file is sourced with error handling at line 26, and the function call at line 109 occurs in the normal execution flow withinmain()with no control flow preventing its execution.docs/CI_CD_PIPELINE.md (1)
37-39: LGTM! Documentation references updated consistently.The script name updates (01_install.sh, 02_uninstall.sh) and the release documentation path updates (development/RELEASE.md) are consistent across all references in the file.
Also applies to: 312-312, 388-388
docs/PRODUCTION_VERSION_FIX.md (1)
1-29: LGTM! Clear documentation of the production version check fix.The document effectively describes the problem (proxy/TLS/timeout issues) and solution (production-ready HTTP client). The environment variable reference provides useful operational guidance.
Note: The static analysis hint about "github.com" capitalization can be ignored - this refers to the API endpoint hostname
api.github.com, which is correctly lowercase.README.md (2)
156-209: LGTM! Excellent CI/CD integration guidance.The new section effectively:
- Warns users that AWS SSO cannot be used in CI/CD (requires browser)
- Provides quick-start examples with non-interactive mode
- Lists supported CI/CD platforms
- Links to comprehensive authentication guide
- Includes a practical GitHub Actions workflow example
426-429: LGTM! Documentation index updated appropriately.The additions of CI/CD Authentication and Version Checking entries improve discoverability of the new documentation.
docs/CI_CD_AUTHENTICATION.md (1)
1-876: LGTM! Comprehensive and well-structured CI/CD authentication guide.This documentation provides excellent coverage of:
- Why SSO doesn't work in CI/CD (clear explanation of OAuth2 Device Authorization Grant limitations)
- Four authentication methods with pros/cons and use cases
- Platform-specific examples (GitHub Actions, GitLab CI, Jenkins, CircleCI, CodeBuild)
- Required IAM permissions and environment variables
- Troubleshooting guidance and best practices
- Security considerations
The content is actionable, accurate, and well-organized with a helpful table of contents and quick reference section.
Optional minor improvements:
- Lines 792, 815: Consider adding language identifier
textto the code fences for the decision trees- Line 876: Consider wrapping the bare URL in angle brackets:
<https://github.com/zsoftly/ztiaws/issues>These are minor style preferences and don't impact the quality of the documentation.
docs/VERSION_CHECKING.md (1)
1-52: LGTM! Clear and concise version checking documentation.The document effectively explains:
- Architecture of the production-ready HTTP client
- Design rationale for proxy support, TLS configuration, retry logic, rate limiting, and error handling
- Environment variables for configuration and troubleshooting
- Implementation references pointing to specific functions in
checker.goThe design decisions section is particularly helpful, as it explains the "why" behind each feature.
Note: The markdown linter complaints about emphasis vs. headings can be ignored - the bold text formatting for subsection labels under "Design Decisions" is appropriate and improves readability.
ztictl/README.md (2)
113-219: LGTM! Comprehensive authentication and CI/CD guidance.The new sections provide excellent coverage:
- Clear distinction between interactive (AWS SSO) and CI/CD (IAM-based) authentication
- Helpful comparison table of authentication methods with security ratings
- Practical examples for OIDC, EC2 Instance Profile, and IAM Access Keys
- Non-interactive mode explanation with clear examples
- Links to complete CI/CD authentication guide
Note: The AI summary mentions "duplication of sections within the README" but this cannot be verified from the diff (which only shows changed lines). You may want to review the full README to ensure the Authentication Methods section doesn't appear multiple times, as duplicate sections reduce documentation clarity.
314-314: LGTM! Release documentation path updated correctly.The path update from
RELEASE.mdto../docs/development/RELEASE.mdaligns with the documentation reorganization seen across other files.ztictl/internal/ssm/manager_test.go (1)
1348-1422: LGTM! Well-designed concurrent initialization tests.The three new tests effectively validate thread-safe initialization:
- TestConcurrentInitializePlatformComponents: Verifies platform component initialization is race-free with 50 concurrent goroutines
- TestConcurrentInitializeManagers: Validates manager initialization safety with 50 concurrent goroutines
- TestConcurrentMixedInitialization: Tests realistic mixed concurrent access with 100 goroutines alternating between initialization methods
Strengths:
- Proper synchronization using buffered channels
- Appropriate use of
t.Skip()for environments without AWS configuration- Sufficient concurrency level (50-100 goroutines) to expose race conditions
- Tests align with the mutex-based concurrency protection added to
manager.go(referenced in PR context)These tests provide good confidence that the concurrent initialization paths are safe for parallel CI/CD operations.
ztictl/cmd/ztictl/auth.go (2)
34-38: LGTM! Documentation updated to warn about CI/CD limitations.The updated Long description appropriately warns users that AWS SSO cannot be used in CI/CD pipelines and points them to the comprehensive CI/CD authentication guide.
43-83: LGTM! Well-implemented CI/CD guard for auth login.The new guard logic effectively prevents AWS SSO login in CI/CD environments:
Strengths:
- Correctly detects non-interactive/CI context using
ExecutionContext- Uses
auth.DetectEnvironmentCredentials()to check for existing IAM credentials- Provides contextually appropriate error messages:
- If credentials exist: tells user they're already configured and to skip
auth login- If no credentials: provides detailed setup instructions with platform-specific examples
- Links to comprehensive CI/CD authentication documentation
- Prevents browser-based SSO flow from executing in automated environments
Flow is sound:
- Check if in CI/CD environment
- Detect credential type (EC2 instance profile, OIDC, IAM keys, etc.)
- Provide appropriate guidance and exit
- Only proceed with
performLogin()in interactive environmentsThis implementation aligns with the broader PR goal of making
ztictlCI/CD-friendly while maintaining security and usability.ztictl/cmd/ztictl/root.go (1)
49-89: Non-interactive env var forces CI mode even when set to “false”
detectCIEnvironmenttreats the mere presence ofZTICTL_NON_INTERACTIVEas proof we’re running in CI. If a user exportsZTICTL_NON_INTERACTIVE=falseto re-enable interactivity,isCIstill returns true,nonInteractiveModeflips on, and every command behaves as if the CLI were in a CI pipeline. That breaks legitimate workflows and contradicts the flag/env semantics introduced here.Drop
ZTICTL_NON_INTERACTIVEfrom the CI detection list (or only treat it as CI when the value is truthy) and continue to rely on the explicit equality check increateExecutionContext.- "CODEBUILD_BUILD_ID", // AWS CodeBuild - "ZTICTL_NON_INTERACTIVE", // Explicit non-interactive mode + "CODEBUILD_BUILD_ID", // AWS CodeBuild @@ - nonInteractiveMode := nonInteractive || - os.Getenv("ZTICTL_NON_INTERACTIVE") == "true" || - isCI + nonInteractiveEnv := strings.EqualFold(os.Getenv("ZTICTL_NON_INTERACTIVE"), "true") + nonInteractiveMode := nonInteractive || nonInteractiveEnv || isCILikely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ztictl/cmd/ztictl/ssm_power_test.go (2)
692-694: Consider simplifying error checks by removing redundantelse.Since the error case returns early, the
elseclause is unnecessary.Apply this pattern:
- } else if err != nil { - t.Errorf("Expected no error but got: %v", err) + } + if err != nil { + t.Errorf("Expected no error but got: %v", err) }Also applies to: 844-846
859-859: Remove trailing blank line.Extra whitespace at the end of the
stubErrortype definition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(3 hunks)docs/examples/github-actions-oidc.yml(1 hunks)docs/examples/gitlab-ci-oidc.yml(1 hunks)ztictl/cmd/ztictl/ssm_power_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
🧰 Additional context used
🧬 Code graph analysis (1)
ztictl/cmd/ztictl/ssm_power_test.go (1)
ztictl/cmd/ztictl/ssm_power.go (1)
PowerOperationResult(349-354)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Go Code (macos-latest)
- GitHub Check: Test Go Code (windows-latest)
- GitHub Check: Test Go Code (ubuntu-latest)
🔇 Additional comments (6)
docs/examples/github-actions-oidc.yml (4)
85-92: Session Manager plugin installation is appropriate for ubuntu-latest.The .deb installer is the correct approach for Ubuntu. The installation, verification, and error handling are properly implemented.
44-46: OIDC permissions are properly scoped.The permission configuration correctly restricts
id-token: write(for OIDC) andcontents: read(for checkout). This follows least-privilege principle without exposing unnecessary permissions.
140-200: Test environment job pattern is well-structured.The use of
if: always()on line 194 to ensure test instances are stopped even on failure is a best practice that prevents resource leaks. The 60-second wait for instance readiness (line 182) is a reasonable default.
264-310: Multi-region matrix strategy is well-implemented.The per-region AWS credential assumption (lines 281-285) with region passed to matrix context is the correct pattern. Health check verification with explicit exit on failure (
|| exit 1) ensures deployment validation.ztictl/cmd/ztictl/ssm_power_test.go (2)
860-926: LGTM! Production helper is now properly tested.This test now calls
buildRequirementsForOperationand validates its output, addressing the concern raised in the past review comment. The test properly verifies all returned fields including operation, SSM online requirement, and allowed states.
1040-1091: Review comment is incorrect—no callable production function exists for this test.The
parseTagFiltersfunctions in bothztictl/internal/ssm/manager.goandztictl/pkg/aws/instance_service.goare private (lowercase, unexported), so they cannot be imported or called from the test file inztictl/cmd/ztictl/ssm_power_test.go. The test file (packagemain) does not import ssm or aws service packages. Additionally, there is no public function calledGetInstanceIDsByTagsin the codebase.This test is appropriately located at the CLI command level and validates tag-parsing behavior for command-line arguments. The actual
parseTagFilterslogic is tested separately in their respective test files (manager_test.goandinstance_service_test.go). The inline implementation here is correct for its architectural layer.Likely an incorrect or invalid review comment.
| # Install AWS Session Manager plugin (tarball for Alpine compatibility) | ||
| - | | ||
| curl "https://s3.amazonaws.com/session-manager-downloads/plugin/latest/linux_64bit/session-manager-plugin.tar.gz" \ | ||
| -o "session-manager-plugin.tar.gz" | ||
| tar -xzf session-manager-plugin.tar.gz | ||
| mv sessionmanager-bundle/bin/session-manager-plugin /usr/local/bin/ | ||
| chmod +x /usr/local/bin/session-manager-plugin | ||
| session-manager-plugin --version |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Session Manager plugin tarball extraction path.
Line 75 assumes the tarball extracts to sessionmanager-bundle/bin/session-manager-plugin. This path needs verification against the actual AWS tarball structure; if the extraction produces a different directory layout, the mv command will fail.
🏁 Script executed:
#!/bin/bash
# Description: Verify AWS Session Manager plugin tarball extraction structure
# Download and extract the tarball to inspect its structure
curl -s "https://s3.amazonaws.com/session-manager-downloads/plugin/latest/linux_64bit/session-manager-plugin.tar.gz" \
-o /tmp/session-manager-plugin.tar.gz
echo "Tarball contents:"
tar -tzf /tmp/session-manager-plugin.tar.gz | head -20
echo ""
echo "Checking for session-manager-plugin binary:"
tar -tzf /tmp/session-manager-plugin.tar.gz | grep -E "session-manager-plugin$|bin/session-manager-plugin" || echo "Not found at expected paths"Length of output: 300
Incorrect Session Manager plugin extraction path on line 75.
The Session Manager plugin binary is located at /usr/local/sessionmanagerplugin/bin/session-manager-plugin on Linux, not at sessionmanager-bundle/bin/session-manager-plugin as the code assumes. The mv command will fail because the source path does not exist, breaking the installation.
Correct line 75 to move from the actual extraction path:
mv ./usr/local/sessionmanagerplugin/bin/session-manager-plugin /usr/local/bin/
🤖 Prompt for AI Agents
In docs/examples/gitlab-ci-oidc.yml around lines 70 to 77, the Session Manager
plugin mv command uses the wrong extracted path and will fail; replace the mv
source path with the actual extraction path
./usr/local/sessionmanagerplugin/bin/session-manager-plugin (i.e., move
./usr/local/sessionmanagerplugin/bin/session-manager-plugin to /usr/local/bin/)
so the binary is relocated correctly and the subsequent chmod and --version
steps succeed.
| func TestPowerOperationResultErrorHandling(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| results []PowerOperationResult | ||
| expectError bool | ||
| description string | ||
| }{ | ||
| { | ||
| name: "state validation error for start on running instance", | ||
| results: []PowerOperationResult{ | ||
| { | ||
| InstanceID: "i-1234567890abcdef0", | ||
| Operation: "start", | ||
| Error: errors.New("instance is in 'running' state, expected one of: [stopped]"), | ||
| Duration: time.Millisecond * 100, | ||
| }, | ||
| }, | ||
| expectError: true, | ||
| description: "Should fail when trying to start a running instance", | ||
| }, | ||
| { | ||
| name: "state validation error for stop on stopped instance", | ||
| results: []PowerOperationResult{ | ||
| { | ||
| InstanceID: "i-1234567890abcdef0", | ||
| Operation: "stop", | ||
| Error: errors.New("instance is in 'stopped' state, expected one of: [running]"), | ||
| Duration: time.Millisecond * 100, | ||
| }, | ||
| }, | ||
| expectError: true, | ||
| description: "Should fail when trying to stop a stopped instance", | ||
| }, | ||
| { | ||
| name: "state validation error for reboot on stopped instance", | ||
| results: []PowerOperationResult{ | ||
| { | ||
| InstanceID: "i-1234567890abcdef0", | ||
| Operation: "reboot", | ||
| Error: errors.New("instance is in 'stopped' state, expected one of: [running]"), | ||
| Duration: time.Millisecond * 100, | ||
| }, | ||
| }, | ||
| expectError: true, | ||
| description: "Should fail when trying to reboot a stopped instance", | ||
| }, | ||
| { | ||
| name: "successful operation with no errors", | ||
| results: []PowerOperationResult{ | ||
| { | ||
| InstanceID: "i-1234567890abcdef0", | ||
| Operation: "start", | ||
| Error: nil, | ||
| Duration: time.Millisecond * 200, | ||
| }, | ||
| }, | ||
| expectError: false, | ||
| description: "Should succeed when instance is in correct state", | ||
| }, | ||
| { | ||
| name: "mixed results with some validation failures", | ||
| results: []PowerOperationResult{ | ||
| { | ||
| InstanceID: "i-1111111111111111", | ||
| Operation: "start", | ||
| Error: nil, | ||
| Duration: time.Millisecond * 150, | ||
| }, | ||
| { | ||
| InstanceID: "i-2222222222222222", | ||
| Operation: "start", | ||
| Error: errors.New("instance is in 'running' state, expected one of: [stopped]"), | ||
| Duration: time.Millisecond * 100, | ||
| }, | ||
| { | ||
| InstanceID: "i-3333333333333333", | ||
| Operation: "start", | ||
| Error: errors.New("instance is in 'running' state, expected one of: [stopped]"), | ||
| Duration: time.Millisecond * 100, | ||
| }, | ||
| }, | ||
| expectError: true, | ||
| description: "Should report partial failure when some instances fail validation", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := displayPowerOperationResults(tt.results, tt.results[0].Operation, time.Second, 4) | ||
|
|
||
| if tt.expectError && err == nil { | ||
| t.Errorf("%s: expected error but got nil", tt.description) | ||
| } | ||
|
|
||
| if !tt.expectError && err != nil { | ||
| t.Errorf("%s: expected no error but got: %v", tt.description, err) | ||
| } | ||
|
|
||
| successCount := 0 | ||
| for _, result := range tt.results { | ||
| if result.Error == nil { | ||
| successCount++ | ||
| } | ||
| } | ||
|
|
||
| if tt.expectError && successCount == len(tt.results) { | ||
| t.Errorf("%s: all operations succeeded but expected failures", tt.description) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add bounds check to prevent panic on empty results.
Line 1016 accesses tt.results[0].Operation without verifying the slice is non-empty. While current test cases all provide at least one result, this creates fragility.
Apply this diff:
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
+ if len(tt.results) == 0 {
+ t.Skip("Test requires at least one result")
+ }
err := displayPowerOperationResults(tt.results, tt.results[0].Operation, time.Second, 4)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestPowerOperationResultErrorHandling(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| results []PowerOperationResult | |
| expectError bool | |
| description string | |
| }{ | |
| { | |
| name: "state validation error for start on running instance", | |
| results: []PowerOperationResult{ | |
| { | |
| InstanceID: "i-1234567890abcdef0", | |
| Operation: "start", | |
| Error: errors.New("instance is in 'running' state, expected one of: [stopped]"), | |
| Duration: time.Millisecond * 100, | |
| }, | |
| }, | |
| expectError: true, | |
| description: "Should fail when trying to start a running instance", | |
| }, | |
| { | |
| name: "state validation error for stop on stopped instance", | |
| results: []PowerOperationResult{ | |
| { | |
| InstanceID: "i-1234567890abcdef0", | |
| Operation: "stop", | |
| Error: errors.New("instance is in 'stopped' state, expected one of: [running]"), | |
| Duration: time.Millisecond * 100, | |
| }, | |
| }, | |
| expectError: true, | |
| description: "Should fail when trying to stop a stopped instance", | |
| }, | |
| { | |
| name: "state validation error for reboot on stopped instance", | |
| results: []PowerOperationResult{ | |
| { | |
| InstanceID: "i-1234567890abcdef0", | |
| Operation: "reboot", | |
| Error: errors.New("instance is in 'stopped' state, expected one of: [running]"), | |
| Duration: time.Millisecond * 100, | |
| }, | |
| }, | |
| expectError: true, | |
| description: "Should fail when trying to reboot a stopped instance", | |
| }, | |
| { | |
| name: "successful operation with no errors", | |
| results: []PowerOperationResult{ | |
| { | |
| InstanceID: "i-1234567890abcdef0", | |
| Operation: "start", | |
| Error: nil, | |
| Duration: time.Millisecond * 200, | |
| }, | |
| }, | |
| expectError: false, | |
| description: "Should succeed when instance is in correct state", | |
| }, | |
| { | |
| name: "mixed results with some validation failures", | |
| results: []PowerOperationResult{ | |
| { | |
| InstanceID: "i-1111111111111111", | |
| Operation: "start", | |
| Error: nil, | |
| Duration: time.Millisecond * 150, | |
| }, | |
| { | |
| InstanceID: "i-2222222222222222", | |
| Operation: "start", | |
| Error: errors.New("instance is in 'running' state, expected one of: [stopped]"), | |
| Duration: time.Millisecond * 100, | |
| }, | |
| { | |
| InstanceID: "i-3333333333333333", | |
| Operation: "start", | |
| Error: errors.New("instance is in 'running' state, expected one of: [stopped]"), | |
| Duration: time.Millisecond * 100, | |
| }, | |
| }, | |
| expectError: true, | |
| description: "Should report partial failure when some instances fail validation", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| err := displayPowerOperationResults(tt.results, tt.results[0].Operation, time.Second, 4) | |
| if tt.expectError && err == nil { | |
| t.Errorf("%s: expected error but got nil", tt.description) | |
| } | |
| if !tt.expectError && err != nil { | |
| t.Errorf("%s: expected no error but got: %v", tt.description, err) | |
| } | |
| successCount := 0 | |
| for _, result := range tt.results { | |
| if result.Error == nil { | |
| successCount++ | |
| } | |
| } | |
| if tt.expectError && successCount == len(tt.results) { | |
| t.Errorf("%s: all operations succeeded but expected failures", tt.description) | |
| } | |
| }) | |
| } | |
| } | |
| func TestPowerOperationResultErrorHandling(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| results []PowerOperationResult | |
| expectError bool | |
| description string | |
| }{ | |
| { | |
| name: "state validation error for start on running instance", | |
| results: []PowerOperationResult{ | |
| { | |
| InstanceID: "i-1234567890abcdef0", | |
| Operation: "start", | |
| Error: errors.New("instance is in 'running' state, expected one of: [stopped]"), | |
| Duration: time.Millisecond * 100, | |
| }, | |
| }, | |
| expectError: true, | |
| description: "Should fail when trying to start a running instance", | |
| }, | |
| { | |
| name: "state validation error for stop on stopped instance", | |
| results: []PowerOperationResult{ | |
| { | |
| InstanceID: "i-1234567890abcdef0", | |
| Operation: "stop", | |
| Error: errors.New("instance is in 'stopped' state, expected one of: [running]"), | |
| Duration: time.Millisecond * 100, | |
| }, | |
| }, | |
| expectError: true, | |
| description: "Should fail when trying to stop a stopped instance", | |
| }, | |
| { | |
| name: "state validation error for reboot on stopped instance", | |
| results: []PowerOperationResult{ | |
| { | |
| InstanceID: "i-1234567890abcdef0", | |
| Operation: "reboot", | |
| Error: errors.New("instance is in 'stopped' state, expected one of: [running]"), | |
| Duration: time.Millisecond * 100, | |
| }, | |
| }, | |
| expectError: true, | |
| description: "Should fail when trying to reboot a stopped instance", | |
| }, | |
| { | |
| name: "successful operation with no errors", | |
| results: []PowerOperationResult{ | |
| { | |
| InstanceID: "i-1234567890abcdef0", | |
| Operation: "start", | |
| Error: nil, | |
| Duration: time.Millisecond * 200, | |
| }, | |
| }, | |
| expectError: false, | |
| description: "Should succeed when instance is in correct state", | |
| }, | |
| { | |
| name: "mixed results with some validation failures", | |
| results: []PowerOperationResult{ | |
| { | |
| InstanceID: "i-1111111111111111", | |
| Operation: "start", | |
| Error: nil, | |
| Duration: time.Millisecond * 150, | |
| }, | |
| { | |
| InstanceID: "i-2222222222222222", | |
| Operation: "start", | |
| Error: errors.New("instance is in 'running' state, expected one of: [stopped]"), | |
| Duration: time.Millisecond * 100, | |
| }, | |
| { | |
| InstanceID: "i-3333333333333333", | |
| Operation: "start", | |
| Error: errors.New("instance is in 'running' state, expected one of: [stopped]"), | |
| Duration: time.Millisecond * 100, | |
| }, | |
| }, | |
| expectError: true, | |
| description: "Should report partial failure when some instances fail validation", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| if len(tt.results) == 0 { | |
| t.Skip("Test requires at least one result") | |
| } | |
| err := displayPowerOperationResults(tt.results, tt.results[0].Operation, time.Second, 4) | |
| if tt.expectError && err == nil { | |
| t.Errorf("%s: expected error but got nil", tt.description) | |
| } | |
| if !tt.expectError && err != nil { | |
| t.Errorf("%s: expected no error but got: %v", tt.description, err) | |
| } | |
| successCount := 0 | |
| for _, result := range tt.results { | |
| if result.Error == nil { | |
| successCount++ | |
| } | |
| } | |
| if tt.expectError && successCount == len(tt.results) { | |
| t.Errorf("%s: all operations succeeded but expected failures", tt.description) | |
| } | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In ztictl/cmd/ztictl/ssm_power_test.go around lines 928 to 1038, the test calls
displayPowerOperationResults(tt.results, tt.results[0].Operation, ...) but does
not check that tt.results is non-empty; add a bounds check before accessing
tt.results[0] — if tt.results is empty, either skip the call (t.Skip or return)
or set a safe default for the operation and call displayPowerOperationResults
accordingly (or assert/expect an error), ensuring no panic occurs when results
is an empty slice.
executePowerOperationParallelto include region parameter for AWS operations.Managerinitialization methods with mutex locks.go.modandgo.sumfor AWS SDK and other packages.Summary by CodeRabbit
New Features
Documentation
Chores