Skip to content

Add helper script to tag EC2 instances#84

Open
dhensel-rh wants to merge 1 commit into
openshift-eng:mainfrom
dhensel-rh:helpers/tag-instance-script
Open

Add helper script to tag EC2 instances#84
dhensel-rh wants to merge 1 commit into
openshift-eng:mainfrom
dhensel-rh:helpers/tag-instance-script

Conversation

@dhensel-rh

@dhensel-rh dhensel-rh commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds helpers/tag-instance.sh to quickly tag EC2 instances with keep-N retention tags
  • Reads instance ID from instance-data/node-0/aws-instance-id automatically
  • Defaults to keep-2 if no argument provided; pass a number for custom days (e.g. ./helpers/tag-instance.sh 5)

Test plan

  • Run ./helpers/tag-instance.sh and verify tag appears on instance
  • Run ./helpers/tag-instance.sh 5 and verify keep-5 tag

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added an instance-tagging helper that applies a “keep-N” AWS tag for short retention (default N=2, validated to 1–5).
    • Added a new deployment tag make target to run the helper and tag the current instance.
  • Documentation
    • Updated deployment help/usage to include the new tag command and how to set DAYS (including default/max behavior).

@openshift-ci openshift-ci Bot requested review from fonta-rh and jaypoulz June 26, 2026 13:07
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

Adds a Bash helper that tags the current EC2 instance with a keep-N retention tag, and wires it into deploy/Makefile as a new target with help text.

Changes

Instance tagging

Layer / File(s) Summary
Tag helper script
helpers/tag-instance.sh
Loads the instance id, resolves the region, validates DAYS, computes keep-${DAYS}, and calls aws ec2 create-tags with that tag key.
Makefile target and help text
deploy/Makefile
Adds a tag target that runs the helper with DAYS and documents the command in the instance utilities help output.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
No-Injection-Vectors ❌ Error deploy/Makefile interpolates $(DAYS) directly into a shell recipe inside single quotes, enabling shell injection if DAYS contains a quote. Escape or validate DAYS before shell expansion, or pass it as an environment variable/positional arg without embedding it in the recipe.
Ai-Attribution ⚠️ Warning PR/commit mention Claude Code, but HEAD only has Co-Authored-By: Claude Opus 4.6 and no Assisted-by/Generated-by trailer. Replace the AI co-author line with a proper Assisted-by: or Generated-by: trailer, and remove Co-Authored-By for AI assistance.
✅ Passed checks (9 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Weak-Crypto ✅ Passed Touched files add an EC2 tagging helper and Makefile target; neither contains weak crypto, custom crypto, or secret comparisons.
Container-Privileges ✅ Passed PR only changes a Makefile and a bash helper; no container/K8s manifests or privileged settings (privileged/hostPID/hostNetwork/etc.) were added.
No-Sensitive-Data-In-Logs ✅ Passed The new script only logs instance ID, region, and retention tag; no passwords, tokens, PII, or other sensitive data are printed.
No-Hardcoded-Secrets ✅ Passed Scanned deploy/Makefile and helpers/tag-instance.sh; found no hardcoded credentials, keys, tokens, passwords, PEMs, URLs with creds, or long base64 literals.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a helper script for tagging EC2 instances.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot added the ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review label Jun 26, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@helpers/tag-instance.sh`:
- Around line 9-11: The tag generation in tag-instance.sh currently accepts any
input for DAYS, so TAG_VALUE can become an invalid keep-* string. Update the
script to validate the first argument is numeric before constructing TAG_VALUE,
and make sure the logic around DAYS and TAG_VALUE only proceeds with a valid
integer so the helper always produces a keep-N style retention tag.
- Around line 7-15: The tagging script currently falls back to a hardcoded
default region, which can silently target the wrong AWS region. Update the
region handling in tag-instance.sh so it requires an explicit AWS region or
derives it from the instance context instead of using the us-east-2 fallback.
Keep the change localized around the REGION assignment and the aws ec2
create-tags call, and ensure the script fails clearly when no valid region is
available.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: a803fe04-efad-443e-9cd1-bf94dcbe2ee4

📥 Commits

Reviewing files that changed from the base of the PR and between 903e071 and 4aee3e4.

📒 Files selected for processing (1)
  • helpers/tag-instance.sh

Comment thread helpers/tag-instance.sh Outdated
Comment thread helpers/tag-instance.sh
Comment thread helpers/tag-instance.sh
Comment thread helpers/tag-instance.sh
@dhensel-rh dhensel-rh force-pushed the helpers/tag-instance-script branch from 4aee3e4 to 2becd74 Compare June 26, 2026 13:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@deploy/Makefile`:
- Around line 86-87: The tag target in the Makefile passes DAYS unquoted to
helpers/tag-instance.sh, which can cause word splitting or shell expansion
before validation. Update the tag recipe so the $(DAYS) value is passed as a
single quoted argument, keeping the command in tag safe even when the variable
contains spaces or special characters.

In `@helpers/tag-instance.sh`:
- Around line 5-6: The tag-instance.sh script currently reads aws-instance-id
directly via cat without checking for a missing or empty file, which leads to a
raw shell failure instead of the standard “no instance found” message. Update
the INSTANCE_ID retrieval logic in the tag flow to mirror the validation used in
deploy/aws-hypervisor/scripts/start.sh by explicitly checking that
INSTANCE_DATA/aws-instance-id exists and is non-empty before reading it, and
emit the repo’s usual user-friendly error if it is absent or blank.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 436efdd2-9cca-48ff-a968-67923843d019

📥 Commits

Reviewing files that changed from the base of the PR and between 4aee3e4 and 2becd74.

📒 Files selected for processing (2)
  • deploy/Makefile
  • helpers/tag-instance.sh

Comment thread deploy/Makefile Outdated
Comment thread helpers/tag-instance.sh
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dhensel-rh dhensel-rh force-pushed the helpers/tag-instance-script branch from 2becd74 to 5c941ce Compare June 27, 2026 15:06
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2026
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2026
@openshift-ci

openshift-ci Bot commented Jun 28, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dhensel-rh, vimauro
Once this PR has been reviewed and has the lgtm label, please assign jerpeter1 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged. ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants