Skip to content

feat(STONEINTG-1519): create PR group snapshots from ComponentGroups#1549

Merged
dirgim merged 4 commits into
konflux-ci:mainfrom
dirgim:STONEINTG-1519
Jun 12, 2026
Merged

feat(STONEINTG-1519): create PR group snapshots from ComponentGroups#1549
dirgim merged 4 commits into
konflux-ci:mainfrom
dirgim:STONEINTG-1519

Conversation

@dirgim

@dirgim dirgim commented Apr 28, 2026

Copy link
Copy Markdown
Member
  • If an incoming build pipeline's component is part of a component group and if the build PLR belongs to a PR group create the group Snapshot based on the componentGroup(s) that the component belongs to
  • Maintain feature parity with the current (application-based) PR group implementation
  • Reduce code complexity for initial integration test status reporting in buildpipeline adapter's EnsureIntegrationTestReportedToGitProvider by extracting the logic into reportIntegrationStatusAndHandleGroups functions for, split by application and componentGroup mode
  • Add a separate loader function for fetching build pipelineRuns for applications
  • Add filterPipelineRunsForComponentGroups that ensures that build pipelineRuns for a PR group also match any of the componentGroups for that component
  • Update unit tests to test for multiple component groups
  • Add unit tests to cover the controller adapters expansion to cover PR groups for componentGroups

Maintainers will complete the following section

@dirgim dirgim force-pushed the STONEINTG-1519 branch 2 times, most recently from 350832c to 5158762 Compare April 28, 2026 14:16
@codecov-commenter

codecov-commenter commented Apr 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.11594% with 132 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.10%. Comparing base (bd638fb) to head (a4c7f02).

Files with missing lines Patch % Lines
.../controller/buildpipeline/buildpipeline_adapter.go 67.85% 30 Missing and 15 partials ⚠️
internal/controller/snapshot/snapshot_adapter.go 72.51% 29 Missing and 7 partials ⚠️
...al/controller/statusreport/statusreport_adapter.go 57.14% 13 Missing and 5 partials ⚠️
loader/loader_mock.go 55.00% 7 Missing and 2 partials ⚠️
loader/loader.go 80.95% 4 Missing and 4 partials ⚠️
snapshot/gcl.go 0.00% 8 Missing ⚠️
gitops/snapshot.go 63.63% 2 Missing and 2 partials ⚠️
snapshot/create.go 50.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1549      +/-   ##
==========================================
+ Coverage   65.62%   73.10%   +7.47%     
==========================================
  Files          63       63              
  Lines        8481     8725     +244     
==========================================
+ Hits         5566     6378     +812     
+ Misses       2271     1693     -578     
- Partials      644      654      +10     
Flag Coverage Δ
e2e-tests 47.33% <29.22%> (?)
unit-tests 66.01% <67.39%> (+0.38%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dirgim dirgim changed the title WIP: feat(STONEINTG-1519): unit tests for PR groups for componentGroups WIP: feat(STONEINTG-1519): create PR group snapshots from ComponentGroups Apr 28, 2026
@dirgim

dirgim commented Apr 29, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for the ComponentGroup model within the snapshot and integration test reporting workflows, facilitating a transition away from the application-centric model. The changes involve significant updates to the build pipeline and snapshot adapters, as well as the underlying object loader. Feedback focuses on critical logic errors in the buildpipeline_adapter.go where scenario counts are overwritten rather than accumulated in loops, and where snapshot annotation logic was incorrectly moved outside of iteration blocks, leading to incomplete reporting. A correction for an error message in the snapshot adapter was also noted to ensure consistency with the new model.

Comment thread internal/controller/buildpipeline/buildpipeline_adapter.go Outdated
Comment thread internal/controller/buildpipeline/buildpipeline_adapter.go Outdated
Comment thread internal/controller/buildpipeline/buildpipeline_adapter.go
Comment thread internal/controller/buildpipeline/buildpipeline_adapter.go
Comment thread internal/controller/snapshot/snapshot_adapter.go Outdated
@dirgim dirgim force-pushed the STONEINTG-1519 branch 2 times, most recently from 12d1c4d to eac2d0f Compare April 29, 2026 09:08
@dirgim

dirgim commented Apr 29, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request extends the PR group snapshot functionality to support the ComponentGroup model alongside the existing Application model. Key changes include refactoring the build pipeline and snapshot adapters to handle both owner types, updating the object loader to use generic owner labels, and exporting snapshot creation helpers. Review feedback identifies a critical isolation issue in the loader where the removal of owner-specific filtering could lead to incorrect snapshot creation. Additionally, performance optimizations are recommended to avoid redundant API calls and inefficient looping when fetching integration test scenarios and processing component lists.

Comment thread loader/loader.go
Comment thread internal/controller/buildpipeline/buildpipeline_adapter.go
Comment thread internal/controller/snapshot/snapshot_adapter.go Outdated
@dirgim dirgim changed the title WIP: feat(STONEINTG-1519): create PR group snapshots from ComponentGroups feat(STONEINTG-1519): create PR group snapshots from ComponentGroups Apr 29, 2026
@dirgim dirgim marked this pull request as ready for review April 29, 2026 11:21

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the integration service to support the ComponentGroup model alongside the existing Application model for snapshot creation and status reporting. Key changes include updating snapshot creation logic, enhancing the object loader to handle both models, and refactoring controllers to use these new capabilities. A suggestion was made to pass slices directly instead of pointers to slices in the fetchSnapshotComponentFromGCL function, as slices are already reference types in Go.

Comment thread internal/controller/snapshot/snapshot_adapter.go Outdated
@dirgim

dirgim commented Apr 29, 2026

Copy link
Copy Markdown
Member Author

/retest

Comment thread internal/controller/buildpipeline/buildpipeline_adapter.go Outdated
@dirgim

dirgim commented Apr 30, 2026

Copy link
Copy Markdown
Member Author

/retest

@jencull jencull 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.

lgtm

Comment thread internal/controller/snapshot/snapshot_adapter.go Outdated
Comment thread internal/controller/snapshot/snapshot_adapter.go
@kasemAlem

Copy link
Copy Markdown
Contributor

Code review

Found 1 issue:

  1. Multiple bare return err without wrapping context in notifySnapshotsInGroupAboutBuild (CLAUDE.md says "Error wrapping: fmt.Errorf("context: %w", err) not %v")

    There are 8 instances of bare return err in this function (lines 721, 732, 742, 749, 757, 774, 781, 796). Errors from GetPipelineRunsWithPRGroupHashForApplication, GetAllApplicationComponents, GetMatchingComponentSnapshotsForComponentAndPRGroupHash, AnnotateSnapshot, and AnnotateBuildPipelineRun are all returned without context, making it hard to trace failures in production. Each should be wrapped, e.g. return fmt.Errorf("failed to get application components: %w", err).

    func (a *Adapter) notifySnapshotsInGroupAboutBuild(pipelineRun *tektonv1.PipelineRun, message string) error {
    prGroupHash := pipelineRun.Labels[gitops.PRGroupHashLabel]
    var allComponentSnapshotsInGroup *[]applicationapiv1alpha1.Snapshot
    var buildPipelineRuns *[]tektonv1.PipelineRun
    if a.application != nil {
    var err error
    buildPipelineRuns, err = a.loader.GetPipelineRunsWithPRGroupHashForApplication(a.context, a.client, a.pipelineRun.Namespace, prGroupHash, a.application.Name)
    if err != nil {
    a.logger.Error(err, fmt.Sprintf("Failed to get build pipelineRuns for given pr group hash %s", prGroupHash))
    return err
    }
    // Don't do anything if the build pipelineRun isn't the latest for its component
    if !tekton.IsLatestBuildPipelineRunInComponent(pipelineRun, buildPipelineRuns) {
    a.logger.Info("not the latest pipelineRun, skipping notifying the group about the failure")
    return nil
    }
    applicationComponents, err := a.loader.GetAllApplicationComponents(a.context, a.client, a.application)
    if err != nil {
    return err
    }
    // Annotate all latest component Snapshots that are part of the PR group
    for _, applicationComponent := range *applicationComponents {
    applicationComponent := applicationComponent // G601
    allComponentSnapshotsInGroup, err = a.loader.GetMatchingComponentSnapshotsForComponentAndPRGroupHash(a.context, a.client,
    a.pipelineRun.Namespace, applicationComponent.Name, prGroupHash, a.application.Name, gitops.ApplicationNameLabel)
    if err != nil {
    a.logger.Error(err, "Failed to fetch Snapshots for component", "component.Name", applicationComponent.Name)
    return err
    }
    if allComponentSnapshotsInGroup != nil && len(*allComponentSnapshotsInGroup) > 0 {
    latestSnapshot := gitops.SortSnapshots(*allComponentSnapshotsInGroup)[0]
    err = gitops.AnnotateSnapshot(a.context, &latestSnapshot, gitops.PRGroupCreationAnnotation,
    message, a.client)
    if err != nil {
    return err
    }
    }
    }
    } else {
    allBuildPipelineRunsWithPRGroupHash, err := a.loader.GetPipelineRunsWithPRGroupHash(a.context, a.client, a.pipelineRun.Namespace, prGroupHash)
    if err != nil {
    a.logger.Error(err, fmt.Sprintf("Failed to get build pipelineRuns for given pr group hash %s", prGroupHash))
    return err
    }
    buildPipelineRuns = a.filterPipelineRunsForComponentGroups(allBuildPipelineRunsWithPRGroupHash, a.componentGroups)
    // Don't do anything if the build pipelineRun isn't the latest for its component
    if !tekton.IsLatestBuildPipelineRunInComponent(pipelineRun, buildPipelineRuns) {
    a.logger.Info("not the latest pipelineRun, skipping notifying the group about the failure")
    return nil
    }
    for _, componentGroup := range *a.componentGroups {
    componentGroup := componentGroup
    for _, groupComponent := range componentGroup.Spec.Components {
    groupComponent := groupComponent
    allComponentSnapshotsInGroup, err = a.loader.GetMatchingComponentSnapshotsForComponentAndPRGroupHash(a.context, a.client,
    a.pipelineRun.Namespace, groupComponent.Name, prGroupHash, componentGroup.Name, gitops.ComponentGroupNameLabel)
    if err != nil {
    a.logger.Error(err, "Failed to fetch Snapshots for component", "component.Name", groupComponent.Name)
    return err
    }
    if allComponentSnapshotsInGroup != nil && len(*allComponentSnapshotsInGroup) > 0 {
    latestSnapshot := gitops.SortSnapshots(*allComponentSnapshotsInGroup)[0]
    err = gitops.AnnotateSnapshot(a.context, &latestSnapshot, gitops.PRGroupCreationAnnotation,
    message, a.client)
    if err != nil {
    return err
    }
    }
    }
    }
    }
    // In case there are in-flight build pipelineRuns, we want to also annotate them to make sure that the failure is propagated
    // to future Snapshots in the group
    for _, buildPipelineRun := range *buildPipelineRuns {
    buildPipelineRun := buildPipelineRun
    // check if build PLR finished
    if !h.HasPipelineRunFinished(&buildPipelineRun) && buildPipelineRun.Labels[tektonconsts.ComponentNameLabel] != a.component.Name {
    err := tekton.AnnotateBuildPipelineRun(a.context, &buildPipelineRun, gitops.PRGroupCreationAnnotation, message, a.client)
    if err != nil {
    return err
    }
    }
    }
    a.logger.Info("notified all component snapshots and build pipelines in the pr group about the build pipeline status",

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:20 AM UTC · Completed 11:34 AM UTC
Commit: dbae38e · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 10, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:49 AM UTC · Completed 12:05 PM UTC
Commit: dbae38e · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See the review comment for full details.

Comment thread status/status.go
Comment thread snapshot/gcl.go
Comment thread internal/controller/snapshot/snapshot_adapter.go
Comment thread loader/loader.go
Comment thread helpers/component_group.go
Comment thread loader/loader.go
@fullsend-ai-review fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Jun 10, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · Started 4:15 PM UTC
Commit: ffde3b2 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See the review comment for full details.

Comment thread internal/controller/buildpipeline/buildpipeline_adapter.go
Comment thread internal/controller/snapshot/snapshot_adapter.go
Comment thread internal/controller/snapshot/snapshot_adapter.go
Comment thread snapshot/create.go
Comment thread loader/loader.go
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:15 PM UTC · Completed 4:31 PM UTC
Commit: ffde3b2 · View workflow run →

@dirgim

dirgim commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

/retest

dirgim added 3 commits June 12, 2026 11:32
* If an incoming build pipeline's component is part of a component group
  and if the build PLR belongs to a PR group create the group Snapshot
  based on the componentGroup(s) that the component belongs to

* Maintain feature parity with the current (application-based)
  PR group implementation

Signed-off-by: dirgim <kpavic@redhat.com>

rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED
* Add unit tests to cover the snapshot adapter's
  expansion to cover PR groups for componentGroups
* Add unit tests to cover the buildpipeline adapter's
  expansion to cover PR groups for componentGroups
* Add unit tests to cover the statusreport adapter's
  expansion to cover PR groups for componentGroups

Signed-off-by: dirgim <kpavic@redhat.com>

rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED
* Reduce code complexity for initial integration test status reporting
  in buildpipeline adapter's EnsureIntegrationTestReportedToGitProvider
  by extracting the logic into reportIntegrationStatusAndHandleGroups
  functions for, split by application and componentGroup mode
* Update unit tests to test for multiple component groups

Signed-off-by: dirgim <kpavic@redhat.com>

rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:35 AM UTC · Completed 9:48 AM UTC
Commit: ffde3b2 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 12, 2026
* add a separate loader function for fetching build pipelineRuns
  for applications
* add filterPipelineRunsForComponentGroups that ensures
  that build pipelineRuns for a PR group also match any of the
  componentGroups for that component
* Optimize fetching snapshot components in snapshot adapter
  when looping over componentGroups
* Add unit tests for the new filtering function and its helper

Signed-off-by: dirgim <kpavic@redhat.com>

Assisted-By: Cursor

rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:56 AM UTC · Completed 10:09 AM UTC
Commit: ffde3b2 · View workflow run →

Comment thread internal/controller/snapshot/snapshot_adapter.go
Comment thread snapshot/gcl.go
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jun 12, 2026
@dirgim

dirgim commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

/retest

@dirgim dirgim merged commit 760e850 into konflux-ci:main Jun 12, 2026
18 checks passed
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jun 12, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 3:29 PM UTC · Completed 3:35 PM UTC
Commit: ffde3b2 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #1549 — feat(STONEINTG-1519): create PR group snapshots from ComponentGroups

Timeline: Human-authored XXL feature PR opened April 28 by dirgim. Four human reviewers approved by May 12. The fullsend review bot began reviewing on June 3 and submitted 4 CHANGES_REQUESTED verdicts (June 3, June 10 ×2, June 11) before finally approving on June 12. The PR merged June 12, ~45 days after opening.

Review bot behavior: The bot reviewed 5 distinct commits (the PR was force-pushed between each round). It raised 11 unique findings across all rounds — 3 medium-severity logic errors and 8 low-severity items (naming conventions, missing godoc comments, edge cases). The medium-severity findings were genuine and were addressed by the author between rounds. Two low-severity findings persisted across rounds (a (nil, nil) return contract documentation gap and a "the the" typo). The bot approved with only 2 low-severity items remaining.

Assessment: The bot's individual findings were reasonable — the medium-severity logic errors in particular were valid catches that the 4 human reviewers missed. However, the use of CHANGES_REQUESTED for rounds containing only low-severity findings added friction to a PR that already had 4 human approvals. The ~1 month gap between human approval (May 12) and merge (June 12) is notable, though it's unclear how much of that delay was caused by the review bot vs other factors.

No new proposals filed. All candidate improvements are already tracked by existing open issues in fullsend-ai/fullsend:

  • CHANGES_REQUESTED for low-severity findings: #2029, #2115, #2057
  • Repeated findings across re-reviews: #1500, #1285
  • Review bot overriding human approvals: #1922
  • Severity calibration and thresholds: #580, #1319

This PR is a good test case for validating fixes to the above issues — a human-authored PR with 4 human approvals where the review bot's value came from catching medium-severity logic errors, but the CHANGES_REQUESTED verdict on low-severity items added unnecessary friction.

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

Labels

ready-for-merge All reviewers approved — ready to merge size: XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants