Skip to content

fix: correct the issues with obtaining pr or branch name #1901

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

Merged
merged 2 commits into from
Feb 4, 2025

Conversation

l-qing
Copy link
Contributor

@l-qing l-qing commented Feb 1, 2025

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).

@l-qing l-qing force-pushed the fix/get-pr-branch-name branch from c75f739 to cf613ef Compare February 1, 2025 07:01
@l-qing
Copy link
Contributor Author

l-qing commented Feb 1, 2025

E2e failure is unrelated to code changes, but is related to the environment being unable to pull the image.

$ docker pull registry.access.redhat.com/ubi9/ubi-micro

Using default tag: latest
Error response from daemon: failed to resolve reference "registry.access.redhat.com/ubi9/ubi-micro:latest": unexpected status from HEAD request to https://registry.access.redhat.com/v2/ubi9/ubi-micro/manifests/latest: 503 Service Unavailable

https://bitbucket.org/cboudjna/pac-e2e-tests/pull-requests/2975/overview

The pod errored with the message: "Back-off pulling image "registry.access.redhat.com/ubi9/ubi-micro": ErrImagePull: failed to pull and unpack image "registry.access.redhat.com/ubi9/ubi-micro:latest": failed to resolve reference "registry.access.redhat.com/ubi9/ubi-micro:latest": unexpected status from HEAD request to https://registry.access.redhat.com/v2/ubi9/ubi-micro/manifests/latest: 503 Service Unavailable."

@zakisk
Copy link
Contributor

zakisk commented Feb 1, 2025

@l-qing yes there is something wrong with registry.access.redhat.com its 503 service unavailable.

@zakisk
Copy link
Contributor

zakisk commented Feb 3, 2025

/test

@chmouel
Copy link
Member

chmouel commented Feb 3, 2025

/retest

@chmouel
Copy link
Member

chmouel commented Feb 3, 2025

/test go-testing

@chmouel
Copy link
Member

chmouel commented Feb 3, 2025

this LGTM to me but can we have a better commit message ,i had to reread multiple time to understand what it does, ie

fix(provider): handle branch names containing command keywords in pipeline comments

This commit fixes a bug in parsing pipeline control comments (e.g., /test, /retest, /cancel) 
when branch names contain command keywords like "/test". 
Previously, the code would incorrectly split comments with such branch names, causing failures.

Key improvements:
- Use `strings.SplitN` to split only on the first instance of the command keyword.
- Add support for comments with key=value pairs after the pipeline name.
- Include test cases for branch names containing "/test" and comments with key=value pairs.

Example:
Before: Branch "feature/test-new" would break parsing due to multiple "/test" splits.
After: The code correctly handles branch names containing command keywords and key=value pairs.

This change makes the comment parsing more robust and reliable for real-world usage.

@chmouel
Copy link
Member

chmouel commented Feb 3, 2025

/ok-to-test

@chmouel
Copy link
Member

chmouel commented Feb 3, 2025

/assign @savitaashture

@chmouel chmouel requested a review from savitaashture February 3, 2025 10:20
@l-qing l-qing force-pushed the fix/get-pr-branch-name branch 4 times, most recently from 0a0aeee to 6083735 Compare February 3, 2025 15:12
@chmouel
Copy link
Member

chmouel commented Feb 3, 2025

/lgtm

we can merge this when e2e ci is green

@l-qing
Copy link
Contributor Author

l-qing commented Feb 4, 2025

/retest

@l-qing
Copy link
Contributor Author

l-qing commented Feb 4, 2025

/test go-testing

This commit fixes a bug in parsing pipeline control comments (e.g., /test,
/retest, /cancel) when branch names contain command keywords like "/test".
Previously, the code would incorrectly split comments with such branch
names, causing failures.

Key improvements:
- Use `strings.SplitN` to split only on the first instance of the command
  keyword.
- Add support for comments with key=value pairs after the pipeline name.
- Include test cases for branch names containing "/test" and comments with
  `key=value` pairs.

Example:
Before: Branch "feature/test-new" would break parsing due to multiple splits
on "/test" keyword.
After: The code now correctly processes branch names with command keywords and
handles key=value pairs.

The parsing is now more robust for real-world scenarios.
@l-qing l-qing force-pushed the fix/get-pr-branch-name branch from 6083735 to 5cd45f2 Compare February 4, 2025 05:55
@chmouel
Copy link
Member

chmouel commented Feb 4, 2025

/retest

@chmouel
Copy link
Member

chmouel commented Feb 4, 2025

@l-qing our cluster is not running during the Europe/Western Asia night 🙇🏻 rerunning it now

@chmouel
Copy link
Member

chmouel commented Feb 4, 2025

/lgtm

Copy link

LGTM Vote Breakdown

Current valid votes: 1/2

User Permission Valid Vote
@chmouel admin

@chmouel chmouel merged commit 1bd4586 into openshift-pipelines:main Feb 4, 2025
6 checks passed
@l-qing l-qing deleted the fix/get-pr-branch-name branch February 5, 2025 07:13
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.

4 participants