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

Preserve labels when updating PR #689

Closed
pauleve opened this issue Dec 31, 2020 · 15 comments · Fixed by #719
Closed

Preserve labels when updating PR #689

pauleve opened this issue Dec 31, 2020 · 15 comments · Fixed by #719
Labels
enhancement New feature or request

Comments

@pauleve
Copy link

pauleve commented Dec 31, 2020

Subject of the issue

Whenever a PR gets updated, its labels are reset to the one for the PR creation.
It would be great to have a way to preserve labels on PR update.

Steps to reproduce

  1. create a PR with labels specified
  2. update labels of the PR manually
  3. update the PR: the labels are reset
@peter-evans
Copy link
Owner

Hi @pauleve

Thanks for opening this issue. I will look at improving the PR update to make this possible.

@peter-evans peter-evans added the enhancement New feature or request label Jan 4, 2021
@AraHaan
Copy link

AraHaan commented Jan 26, 2021

@peter-evans I would like this as an optional thing because when it updates the pr I would like to be able to set if I want the labels reset or not (in which case I do because I then use an labeler action when build passes so then automerge could do it's job only when certain conditional things are true).

Or perhaps if one does not want all labels to stay maybe they should list the specific labels that it wants this action to remove from the pull request.

@peter-evans
Copy link
Owner

@AraHaan Interesting use case, thank you for sharing.

I'm still thinking about how to implement this because this problem affects not just labels, but assignees, reviewers and team-reviewers, too. I was intending to just default them all to preserving rather than overwriting, but perhaps labels at least should have the option to reset. I can't think of a good use case for resetting assignees, reviewers and team-reviewers, though.

It's tricky to handle this well in a tidy way. I'm being very careful to not complicate the action and add too many configuration options.

@peter-evans
Copy link
Owner

After giving this a lot of thought, I've decided to change the way the list type fields of the pull request are updated by the action. Rather than reset them, I want to default them to preserving any list content that already exists. From a UX perspective I think it's unexpected and not a good experience for the action to reset these fields, and it almost could be viewed as a bug. It was never my intention to support workflows that rely on the reset behaviour of list fields.

@AraHaan What this means is that the action won't reset the labels field, and I don't think I want to complicate the action with further input options to support this. However, I will help you to retain the existing behaviour in a different way. Please could you show me your workflow so I can see how you are using this action in combination with the labeler action. I'm fairly sure it will be possible to reset the labels as you need with an additional workflow step.

@AraHaan
Copy link

AraHaan commented Feb 1, 2021

sure https://github.com/Elskom/Sdk/ the workflows are under .github/workflows/.

@pauleve
Copy link
Author

pauleve commented Feb 1, 2021

Thanks for your work on this issue!
I agree that it is quite unexpected that the fields (and description?) get reset on update. I guess that the reset operations can be performed by another action reacting to a push on a pull request.

@peter-evans
Copy link
Owner

@pauleve Just to clarify, it is intentional that fields such as title and body are updated by the action because those are fields that I would expect users not to edit manually. I think this behaviour by the action can be reasonably expected. However, the list type fields (labels, assignees, reviewers, team-reviewers) I now realise should be preserving any additional content that has been manually added to them. I don't think it's expected behaviour that additional content added is reset. So my plan is to change the list type fields only, and preserve their contents on update.

@peter-evans
Copy link
Owner

@AraHaan I see that you already added a step to your workflow to remove the automerge label when the pull request is updated. I think that will work fine as an alternative to this action's reset behaviour. 👍

https://github.com/Elskom/Sdk/blob/main/.github/workflows/dotnetcore.yml#L12-L18

@AraHaan
Copy link

AraHaan commented Feb 6, 2021

@AraHaan I see that you already added a step to your workflow to remove the automerge label when the pull request is updated. I think that will work fine as an alternative to this action's reset behaviour. 👍

https://github.com/Elskom/Sdk/blob/main/.github/workflows/dotnetcore.yml#L12-L18

True, but it is possibly best to extract it to another yml file.

@peter-evans
Copy link
Owner

On closer inspection this issue only affects labels and assignees. The fields reviewers and team-reviewers are not reset as I originally thought.

@peter-evans
Copy link
Owner

peter-evans commented Feb 8, 2021

@AraHaan This might be a good alternative for you. You can remove the automerge label straight after this action updates the pull request.

    - name: Create Pull Request
      id: cpr
      uses: peter-evans/create-pull-request@v3

    - uses: actions-ecosystem/action-remove-labels@v1
      if: steps.cpr.outputs.pull-request-operation == 'updated'
      continue-on-error: true
      with:
        github_token: ${{ secrets.GITHUB_TOKEN }}
        labels: |
          automerge

@peter-evans
Copy link
Owner

Released as v3.8.1 / v3.

@pauleve Sorry it took a while to resolve this. Thank you for reporting.

@AraHaan
Copy link

AraHaan commented Feb 8, 2021

Also @peter-evans remove labels can sometimes throw errors which then need to be handled to not fail the workflow (continue-on-error).

@arbourd
Copy link

arbourd commented Feb 9, 2021

It seems that assignees functionality does not work any longer with 3.8.0 or 3.8.1. I can see that the user is still being assigned in the output of this action, but is not assigned on the PR itself. No errors. Works fine when I locked to 3.7.0.

@peter-evans
Copy link
Owner

peter-evans commented Feb 10, 2021

@arbourd Apologies. I introduced a bug with assignees in v3.8.1. I've just fixed it and released v3.8.2.
Thank you for reporting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants