Skip to content
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

fix: remove the constraint on branch name #15

Merged

Conversation

fleboulch
Copy link
Contributor

@fleboulch fleboulch commented May 30, 2024

This constraint breaks the commits interaction on git branches with a /

Steps to reproduce

  1. create a branch with a / (example: feat/my-branch)
  2. add some cool stuff and commit them
  3. push and open an MR
  4. in slack: /homer review !<ID>
  5. add a new commit
  6. you should not receive any message in the thread (whereas you should receive one)

@fleboulch fleboulch force-pushed the contrib/fix-branch-name-constraint branch from 95d703c to 73a156e Compare May 31, 2024 07:04
@fleboulch fleboulch requested a review from greg0ire May 31, 2024 08:41
@greg0ire
Copy link
Member

Is it possible to add a test (or change existing tests so that they fail without your change?)

@fleboulch fleboulch force-pushed the contrib/fix-branch-name-constraint branch from 73a156e to be2c6c8 Compare June 10, 2024 10:27
@fleboulch
Copy link
Contributor Author

I added a new test

@greg0ire greg0ire requested a review from pfongkye June 10, 2024 12:22
@fleboulch fleboulch requested a review from pfongkye June 12, 2024 19:30
@greg0ire greg0ire closed this Jun 27, 2024
@greg0ire greg0ire reopened this Jun 27, 2024
@greg0ire
Copy link
Member

Don't mind me, just trying to trigger a pipeline with the brand new coverage action contributed in #25

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@876569b). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #15   +/-   ##
=======================================
  Coverage        ?   76.78%           
=======================================
  Files           ?       64           
  Lines           ?     1189           
  Branches        ?      209           
=======================================
  Hits            ?      913           
  Misses          ?      272           
  Partials        ?        4           

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

@fleboulch
Copy link
Contributor Author

@pfongkye did you have some time to check this MR?

@pfongkye
Copy link
Contributor

pfongkye commented Jul 3, 2024

@pfongkye did you have some time to check this MR?

Hello,
Yes I left two comments. If you can take a look. Apart from this, LGTM.

@fleboulch
Copy link
Contributor Author

fleboulch commented Jul 3, 2024

I replied to both of them. If you agree with them I can resolve them?

@pfongkye
Copy link
Contributor

pfongkye commented Jul 3, 2024

I replied to both of them. If you agree with them I can resolve them?

Sorry @fleboulch but I'm not seeing your replies for this and this 🙏

@fleboulch
Copy link
Contributor Author

They were in pending state... 💥

@greg0ire greg0ire merged commit 78e8e5b into ManoManoTech:main Jul 3, 2024
2 checks passed
@greg0ire
Copy link
Member

greg0ire commented Jul 3, 2024

Thanks @fleboulch !

@fleboulch fleboulch deleted the contrib/fix-branch-name-constraint branch September 26, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants