Skip to content

feat: support PipelineRun running status reporting with Kueue #2120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zakisk
Copy link
Contributor

@zakisk zakisk commented Jun 10, 2025

This adds support running status of the PipelineRun when PipelineRuns are managed by Kueue.

https://issues.redhat.com/browse/SRVKP-7738

Changes

Submitter Checklist

  • 📝 Ensure your commit message is clear and informative. Refer to the How to write a git commit message guide. Include the commit message in the PR body rather than linking to an external site (e.g., Jira ticket).

  • ♽ Run make test lint before submitting a PR to avoid unnecessary CI processing. Consider installing pre-commit and running pre-commit install in the repository root for an efficient workflow.

  • ✨ We use linters to maintain clean and consistent code. Run make lint before submitting a PR. Some linters offer a --fix mode, executable with make fix-linters (ensure markdownlint and golangci-lint are installed).

  • 📖 Document any user-facing features or changes in behavior.

  • 🧪 While 100% coverage isn't required, we encourage unit tests for code changes where possible.

  • 🎁 If feasible, add an end-to-end test. See README for details.

  • 🔎 Address any CI test flakiness before merging, or provide a valid reason to bypass it (e.g., token rate limitations).

  • If adding a provider feature, fill in the following details:

    • GitHub App
    • GitHub Webhook
    • Gitea/Forgejo
    • GitLab
    • Bitbucket Cloud
    • Bitbucket Data Center

    (update the provider documentation accordingly)

@zakisk zakisk marked this pull request as draft June 10, 2025 09:51
@zakisk zakisk force-pushed the SRVKP-7738-support-running-status-for-kueue branch 3 times, most recently from 81c9dbc to c139528 Compare June 13, 2025 10:47
@zakisk zakisk marked this pull request as ready for review June 13, 2025 10:47
@zakisk zakisk requested a review from chmouel June 13, 2025 10:47
@zakisk zakisk changed the title feat: support PipelineRun running status reporting on Github with Kueue feat: support PipelineRun running status reporting with Kueue Jun 13, 2025
@zakisk zakisk force-pushed the SRVKP-7738-support-running-status-for-kueue branch from c139528 to 7495826 Compare June 13, 2025 14:59
@pramodbindal
Copy link

PR looks good to me.
When can expect this PR available in nightly ?

@zakisk
Copy link
Contributor Author

zakisk commented Jun 16, 2025

PR looks good to me. When can expect this PR available in nightly ?

@pramodbindal after merge, it will be in nightly

@pramodbindal
Copy link

PR looks good to me. When can expect this PR available in nightly ?

@pramodbindal after merge, it will be in nightly

:-) My question was that only. when are you planning to merge

@zakisk
Copy link
Contributor Author

zakisk commented Jun 16, 2025

waiting for reviewers 🙂

@zakisk zakisk force-pushed the SRVKP-7738-support-running-status-for-kueue branch 2 times, most recently from d7ef4b1 to ecff26c Compare June 17, 2025 05:34
@chmouel
Copy link
Member

chmouel commented Jun 19, 2025

🤖 AI Review provided by openrouter:deepseek/deepseek-r1:free:

The analysis of the provided code changes reveals a few areas that need attention:

Bugs and Issues:

  1. Test Does Not Verify Labels:
    • Location: pipelineascode_test.go
    • Issue: The tests check the queued state in annotations but do not verify the corresponding labels. The code adds both annotations and labels, so the tests should validate both to ensure correctness.
    • Fix: Update tests to check both annotations and labels. For example:
      stateAnnotation, ok := pr.GetAnnotations()[keys.State]
      assert.Assert(t, ok, "State annotation missing")
      assert.Equal(t, stateAnnotation, kubeinteraction.StateQueued)
      stateLabel, ok := pr.GetLabels()[keys.State]
      assert.Assert(t, ok, "State label missing")
      assert.Equal(t, stateLabel, kubeinteraction.StateQueued)

Spelling Mistake:

  • Commit Message Typo:
    • Issue: The commit message has "Github" instead of the correct "GitHub".
    • Fix: Amend the commit message to use "GitHub".

Code Improvements:

  1. Test Coverage for Merge Patch:
    • Location: pipelineascode_test.go (TestGetMergePatch)
    • Improvement: The test for getMergePatch doesn't check label patching. Add a case to verify both annotations and labels are patched.
      func TestGetMergePatchWithLabels(t *testing.T) {
          ann := map[string]string{keys.State: kubeinteraction.StateQueued}
          labels := map[string]string{keys.State: kubeinteraction.StateQueued}
          result := getMergePatch(ann, labels)
          m, ok := result["metadata"].(map[string]any)
          assert.Assert(t, ok)
          a, ok := m["annotations"].(map[string]string)
          assert.Assert(t, ok)
          assert.Equal(t, a[keys.State], kubeinteraction.StateQueued)
          l, ok := m["labels"].(map[string]string)
          assert.Assert(t, ok)
          assert.Equal(t, l[keys.State], kubeinteraction.StateQueued)
      }

Code Style and Clarity:

  • Function Naming: The function getMergePatch is functional, but a more descriptive name like createMetadataPatch could enhance readability.
  • Comments: Clarify that the reconciler (watcher) uses the state annotations and labels to track PipelineRun status for reporting to the Git provider.

Summary: The code effectively adds queued state reporting but requires test adjustments to ensure labels are verified, a minor typo fix, and enhanced test coverage for patches.

@chmouel
Copy link
Member

chmouel commented Jun 19, 2025

I have tested it and it worked very nicely

https://streamable.com/3wrn4a

Can you provide some documentation to point that PAC is compatible with kueue and that it can provide an alternative to our concurrency system.. You don't need to go over the whole setup process...

@chmouel
Copy link
Member

chmouel commented Jun 19, 2025

@pramodbindal we don't merge our own PR in PAC, unless we have approved reviewers, and this should be the standard for every openshift-pipelines projects

@zakisk
Copy link
Contributor Author

zakisk commented Jun 19, 2025

Can you provide some documentation to point that PAC is compatible with kueue and that it can provide an alternative to our concurrency system.. You don't need to go over the whole setup process...

@chmouel okay, will do. thanks!

@zakisk zakisk force-pushed the SRVKP-7738-support-running-status-for-kueue branch 2 times, most recently from bbd0113 to d5c0531 Compare June 20, 2025 10:32
@zakisk
Copy link
Contributor Author

zakisk commented Jun 20, 2025

@chmouel PTAL.

this adds support running status of the PipelineRun when
PipelineRuns are managed by Kueue.

https://issues.redhat.com/browse/SRVKP-7738

Signed-off-by: Zaki Shaikh <[email protected]>
@zakisk zakisk force-pushed the SRVKP-7738-support-running-status-for-kueue branch from d5c0531 to 3fbb75d Compare June 21, 2025 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants