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

Copy labels as specified by copy_labels_pattern #314

Closed
wants to merge 1 commit into from
Closed

Copy labels as specified by copy_labels_pattern #314

wants to merge 1 commit into from

Conversation

lheckemann
Copy link
Contributor

@lheckemann lheckemann commented Dec 26, 2022

Fixes #87

@lheckemann
Copy link
Contributor Author

lheckemann commented Dec 26, 2022

@korthout I tried rebasing on main to fix the merge conflict, but it appears that `main` doesn't currently build:
$ npm run build

> [email protected] build
> tsc

src/git.ts:1:10 - error TS2616: 'execa' can only be imported by using 'import execa = require("execa")' or a default import.

1 import { execa } from "execa";
           ~~~~~


Found 1 error in src/git.ts:1

Copy link
Owner

@korthout korthout left a comment

Choose a reason for hiding this comment

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

Thanks so much @lheckemann 🚀 Such a cool feature. I'm sure my team wants to use this as well.

🙇 I have 2 small change requests, and then we should be able to merge this. Let me know if you want to apply these, or whether I should take it over. Happy to do so.

I tried rebasing on main to fix the merge conflict, but it appears that main doesn't currently build

The exaca dependency was updated to a new major version with a breaking change (the import is now named, instead of default). Running npm ci locally should solve the issue for you. You can then build a new version with npm run all.

Comment on lines +72 to +73
# Regex pattern to match github labels which will be copied from the
# original PR to the backport PR.
Copy link
Owner

Choose a reason for hiding this comment

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

Let's clarify what happens to the backport labels. Also please note that the documentation generally uses the term pull request over PR.

Suggested change
# Regex pattern to match github labels which will be copied from the
# original PR to the backport PR.
# Regex pattern to match github labels which will be copied from the
# original pull request to the backport pull request. Labels matching
# `label_pattern` will not be copied.

Please also apply these changes to action.yml

# Optional
# Regex pattern to match github labels which will be copied from the
# original PR to the backport PR.
# copy_labels_pattern: ^severity.*
Copy link
Owner

Choose a reason for hiding this comment

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

The examples show the default value. I'd like to continue that.

Suggested change
# copy_labels_pattern: ^severity.*
# copy_labels_pattern: null

Copy link
Contributor Author

@lheckemann lheckemann Dec 29, 2022

Choose a reason for hiding this comment

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

Can we still put a "useful" example somewhere visible?

Copy link
Owner

Choose a reason for hiding this comment

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

Sure. How about in the comment which describes the field?

@korthout
Copy link
Owner

@lheckemann Would you like me to take it over from here, or do you want to work on the requested changes yourself?

@lheckemann
Copy link
Contributor Author

lheckemann commented Jan 11, 2023

I noticed another problem: the default of null probably doesn't trigger the no-regex-defined logic and will instead probably result in all labels being copied. I dug around in this a couple days ago and didn't really get that far, partly for lack of familiarity with GHA, so if you're willing to take that over I'd appreciate it! Otherwise I'm sure I'll get around to bashing my head against this again within a couple of weeks.

@korthout
Copy link
Owner

My apologies, @lheckemann. I've prioritized #317 because it was already fully functional (and smaller in scope). I'll be out of the office next week, but I'll continue with this pull request as soon as I find the time again.

@korthout
Copy link
Owner

korthout commented Feb 14, 2023

@lheckemann I finally found some time to look into this.

I've just pushed a new branch korthout:copy-labels rebasing your PR on top of main and potentially fixing the problem you described above.

Next, I want to run some tests with this to see if all works well. But it looks good so far! 🎉 Thanks again for this!

@lheckemann
Copy link
Contributor Author

Oh, that's a simple solution! Thanks for taking care of it.

@korthout
Copy link
Owner

Just finished testing ✅ I needed to make a few small adjustments, but it works:

Case 1

Case 2:

I'll clean up the commits and open a new PR (as I cannot edit this one).

@korthout
Copy link
Owner

I had to shift focus a bit before I could release it, but it's released in v1.2.0 🚀

Thanks again for all your efforts @lheckemann 🥇

@lheckemann lheckemann deleted the copy-labels branch February 23, 2023 14: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.

Port origin PR labels to backport PR
2 participants