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

Allow passing a base branch that doesn't have version info #70

Merged
merged 13 commits into from
Nov 16, 2024

Conversation

Jackenmen
Copy link
Contributor

@Jackenmen Jackenmen commented Jul 10, 2022

Resolves #69

I could add enforcement of something here if this is too lax but I'm not sure what kind of check that could be. I suppose I could also add an option to .cherry_picker.toml that needs to be explicitly enabled to allow passing non-version branches if that makes it any better.

I'm aware this issue wasn't accepted but I kinda felt like doing it anyway, even if it doesn't end up merged :P

@Mariatta
Copy link
Member

Mariatta commented Oct 4, 2022

I think this change is reasonable.

@Jackenmen
Copy link
Contributor Author

Tests are currently failing on main and so this PR is affected, I fixed them in #76.

@Mariatta Mariatta added the hacktoberfest-accepted Accepted for hacktoberfest label Oct 4, 2022
@Mariatta
Copy link
Member

Mariatta commented Oct 4, 2022

I think allowing it to be configured via the toml file is a good idea as well. Would you be able to add that in a separate PR?

@Jackenmen
Copy link
Contributor Author

I can do it here or in a separate PR, whatever you prefer. Can probably get to it tomorrow.

I assume you would want this to be a true/false setting determining whether branches without version info are allowed (defaulting to false to keep the current behavior intact)?

@Mariatta
Copy link
Member

Mariatta commented Oct 4, 2022

I think it should support the original behavior if not supplied.

@Jackenmen
Copy link
Contributor Author

Jackenmen commented Oct 5, 2022

Alright, I added a require_version_in_branch_name option. Quite a mouthful but I couldn't really come up with anything else that was still clear about what the option is. At most I could maybe manage to make the name 4-5 characters shorter but the names I came up with were, in my opinion, a lot more vague than the current name.


While looking into this, I also realized that I suggested adding such an option in the PR's description which I didn't really realize when you asked me to add it 😄

cherry_picker/cherry_picker.py Outdated Show resolved Hide resolved
cherry_picker/cherry_picker.py Outdated Show resolved Hide resolved
Jackenmen added a commit to Cog-Creators/Red-GitHubBot that referenced this pull request Aug 27, 2023
Jackenmen added a commit to Cog-Creators/Red-GitHubBot that referenced this pull request Aug 27, 2023
@Jackenmen
Copy link
Contributor Author

I created python/miss-islington#639 to make miss-islington work with this PR since I realized that this is going to be necessary now that this also adds a new configuration option.

@Jackenmen
Copy link
Contributor Author

I addressed the review and resolved the merge conflicts. Seems like I may have missed the merge window for the release though 😄

If by the time this is looked at by a maintainer new conflicts arise, just let me know and I'll resolve them - maybe I'll finally be able to use upstream directly :)

@webknjaz
Copy link
Contributor

@hugovk mind taking a look?

@hugovk
Copy link
Member

hugovk commented Nov 13, 2024

@Mariatta @ezio-melotti Any further comments?

@gopidesupavan
Copy link

Thank you @Jackenmen for creating this; it's really helpful. In Airflow, we're working on automating backporting. We attempted to use cherry-picker, but due to version requirements in the base branch, it isn't feasible. This is because our Apache Airflow project uses slightly different branch naming conventions (ex: v2-1-test, v3-1-stable etc;) .

@hugovk Is there a plan to release this in the next cycle? If so, could you please share when that might be?

Copy link
Member

@Mariatta Mariatta left a comment

Choose a reason for hiding this comment

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

Thank you

@Jackenmen
Copy link
Contributor Author

I'm glad to learn I'm not the only person that finds this feature useful 😄 Have you tried installing from this PR to see that it definitely works for your use case? Might be worth doing considering that cherry-picker doesn't have a very regular release schedule.

@gopidesupavan
Copy link

I'm glad to learn I'm not the only person that finds this feature useful 😄 Have you tried installing from this PR to see that it definitely works for your use case? Might be worth doing considering that cherry-picker doesn't have a very regular release schedule.

Yes i have tested it is working :) . cool

@potiuk
Copy link
Contributor

potiuk commented Nov 16, 2024

Indeed you are not the only one @Jackenmen it would be great to merge this one, we really would love to use it in Airflow. @uranusjr - one of the PyPI maintainers and our fellow PMC member recommended it to us. Also we checked a number of other solutions and cherry-picker only lacks this particular feature and @gopidesupavan had already tested that it would work for for Apache Airflow. And we are now considering forking the project rather than installing from PyPI. So it would be great to get this one merged and released.

@hugovk @ezio-melotti @ambv - Is there anything we can help with to make it happen :) ? Or maybe there are some concerns with merging that one ?

@hugovk
Copy link
Member

hugovk commented Nov 16, 2024

I'll merge and release later today :)

@potiuk
Copy link
Contributor

potiuk commented Nov 16, 2024

🙇 🙇 🙇 🙇

@webknjaz
Copy link
Contributor

webknjaz commented Nov 16, 2024

@potiuk I can also offer using my Patchback bot that can work as an automatic companion to this semi-annual CLI tool FYI.
Had I known earlier, I would've shown it to you on Saturday when we were in the same room...

Examples: pytest-dev/pytest#12912 (comment) / aio-libs/aiohttp#9852 (comment).

Feel free to ping me in a separate issue or on slack if interested, so we don't cause unnecessary notifications here..

@hugovk hugovk merged commit 940787a into python:main Nov 16, 2024
22 checks passed
@potiuk
Copy link
Contributor

potiuk commented Nov 16, 2024

@webknjaz our world is small :) . I think we have certain limitations int the Apache Software Foundation and 3rd-party bots that can update content are generally not approved by the infrastructure. Of course details matters and maybe @gopidesupavan can take a look at it :)

@Jackenmen Jackenmen deleted the issue/69 branch November 16, 2024 10:56
hugovk pushed a commit to hugovk/cherry-picker that referenced this pull request Nov 16, 2024
Co-authored-by: Ezio Melotti <[email protected]>
Co-authored-by: jack1142 <[email protected]>
Co-authored-by: Mariatta Wijaya <[email protected]>
@webknjaz
Copy link
Contributor

@potiuk yeah, I figured you might want to run own instance, which is entirely possible 🤷‍♂️

@potiuk
Copy link
Contributor

potiuk commented Nov 16, 2024

@potiuk yeah, I figured you might want to run own instance, which is entirely possible 🤷‍♂️

Yep. We might do just that.

@gopidesupavan
Copy link

@webknjaz our world is small :) . I think we have certain limitations int the Apache Software Foundation and 3rd-party bots that can update content are generally not approved by the infrastructure. Of course details matters and maybe @gopidesupavan can take a look at it :)

Yeah i will have a look.

@hugovk
Copy link
Member

hugovk commented Nov 16, 2024

Released as 2.4.0 🍒🚀

@potiuk
Copy link
Contributor

potiuk commented Nov 16, 2024

Released as 2.4.0 🍒🚀

Fantastic! Many thanks!

@gopidesupavan
Copy link

Released as 2.4.0 🍒🚀

Woohoo thank you. Appreciate your help and support 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted for hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider laxing the requirements for maintenance branch names?
7 participants