CNTRLPLANE-3763: tag bastion resources with e2e provenance#9022
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@ironcladlou: This pull request references CNTRLPLANE-3763 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
📝 WalkthroughWalkthroughThe AWS bastion creation command adds an Sequence Diagram(s)sequenceDiagram
participant E2ESetup
participant BastionCreate
participant AWSResources
participant EC2
E2ESetup->>BastionCreate: provide source and Prow job tags
BastionCreate->>BastionCreate: parse key=value tags
BastionCreate->>AWSResources: pass additional tags
AWSResources->>EC2: apply tags to resource specifications
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
openshift-ci
Bot
added
area/cli
The bastion keypair, security group, and EC2 instance were missed in PR openshift#8909. Plumb AdditionalTags through CreateBastionOpts so the e2e journal dump path tags them with source and prow-job-id.
5ba9d86 to
9c3de5d
Compare
ironcladlou
changed the title
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, ironcladlou The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/bastion/aws/create.go (1)
166-172: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePreallocate slice capacity.
Since the size of
tagMapis known, preallocate the slice capacity to avoid unnecessary allocations, as per the coding guidelines.♻️ Proposed refactor
- var additionalTags []ec2types.Tag + additionalTags := make([]ec2types.Tag, 0, len(tagMap)) for k, v := range tagMap { additionalTags = append(additionalTags, ec2types.Tag{ Key: aws.String(k), Value: aws.String(v), }) }🤖 Prompt for 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. In `@cmd/bastion/aws/create.go` around lines 166 - 172, Update the additionalTags slice initialization in the tagMap loop to preallocate capacity using len(tagMap), while preserving the existing tag conversion and append behavior.Source: Coding guidelines
test/e2e/util/dump/journals.go (1)
155-160: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePreallocate slice capacity.
To avoid a potential reallocation when appending the Prow job ID tag, consider preallocating the slice capacity to 2 as per the coding guidelines.
♻️ Proposed refactor
- additionalTags := []string{ - supportawsutil.HypershiftSourceTagKey + "=e2e", - } + additionalTags := make([]string, 0, 2) + additionalTags = append(additionalTags, supportawsutil.HypershiftSourceTagKey+"=e2e") if prowJobID := os.Getenv("PROW_JOB_ID"); prowJobID != "" { additionalTags = append(additionalTags, supportawsutil.HypershiftProwJobIDTagKey+"="+prowJobID) }🤖 Prompt for 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. In `@test/e2e/util/dump/journals.go` around lines 155 - 160, Preallocate capacity for two elements when initializing additionalTags in the journal dump flow, while preserving the existing initial Hypershift source tag and conditional Prow job ID append behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@cmd/bastion/aws/create.go`:
- Around line 166-172: Update the additionalTags slice initialization in the
tagMap loop to preallocate capacity using len(tagMap), while preserving the
existing tag conversion and append behavior.
In `@test/e2e/util/dump/journals.go`:
- Around line 155-160: Preallocate capacity for two elements when initializing
additionalTags in the journal dump flow, while preserving the existing initial
Hypershift source tag and conditional Prow job ID append behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bdbe69b2-7324-4775-bcdd-17e7e56d0575
📒 Files selected for processing (2)
cmd/bastion/aws/create.gotest/e2e/util/dump/journals.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9022 +/- ##
==========================================
- Coverage 44.17% 44.17% -0.01%
==========================================
Files 772 772
Lines 96333 96346 +13
==========================================
+ Hits 42558 42559 +1
- Misses 50831 50843 +12
Partials 2944 2944
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/verified by e2e for regressions and manual testing to verify tag support |
openshift-ci-robot
added
the
verified
|
@ironcladlou: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/override codecov/patch |
|
/override codecov/project |
|
@ironcladlou: ironcladlou unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers openshift-sustaining-engineers. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@ironcladlou: ironcladlou unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers openshift-sustaining-engineers. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
Test Resultse2e-aws
e2e-aks
|
|
/retest-required |
|
/override codecov/patch |
|
@csrwng: Overrode contexts on behalf of csrwng: codecov/patch, codecov/project DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@ironcladlou: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
85c46e8
into
openshift:main
5 participants
The bastion keypair, security group, and EC2 instance were missed
in PR #8909. Plumb AdditionalTags through CreateBastionOpts so
the e2e journal dump path tags them with source and prow-job-id.
Summary by CodeRabbit
key=valuetags when creating AWS bastions.