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

feat: add assignees option to the copy and backport commands #2336

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

v1v
Copy link
Contributor

@v1v v1v commented Mar 30, 2021

Description of the change

Add assignees option to the copy and backport commands

Related issues

Fix #2287

Checklists

Development

  • All checks must pass (Semantic Pull Request, pep8, requirements, unit tests, functional tests, security checks, …)
  • The code changed/added as part of this pull request must be covered with tests
  • Hotfixes must have a link to Sentry issue and the hotfix label
  • Features must have a link to a Linear task

Code Review

Code review policies are handled and automated by Mergify.

  • When all tests pass, reviewers will be assigned automatically.
  • When change is approved by at least one review and no pending review are
    remaining, pull request is retested against its base branch.
  • The pull request is then merged automatically.

@v1v
Copy link
Contributor Author

v1v commented Mar 30, 2021

Similar to #2334.

I've no clue how to test this locally, though my changes in the test data were good enough 👑

@mergify mergify bot requested a review from a team March 30, 2021 12:44
@v1v v1v marked this pull request as ready for review March 30, 2021 13:26
Copy link
Member

@sileht sileht left a comment

Choose a reason for hiding this comment

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

Maybe we can be more generic like assign action does, for example:

voluptuous.Required("assignees", default[]: [types.Jinja2])

And then the configuration will look like this:

backport:
    assignees: ["{{ author }}"]

What do you think?

@mergify mergify bot requested a review from a team April 2, 2021 08:07
@mergify
Copy link
Contributor

mergify bot commented Apr 2, 2021

@v1v this pull request is now in conflict 😩

@mergify mergify bot added conflict and removed conflict labels Apr 2, 2021
Comment on lines 66 to 72
f.write(
f"""<head>
<meta http-equiv="refresh" content="0; URL={url}">
<link rel="canonical" href="{url}">
</head>
""")
"""
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated change caused by the tox -e black command

Copy link
Member

Choose a reason for hiding this comment

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

I will fix the tox target

@v1v v1v force-pushed the feature/add-assignee-author-backport branch from e646b00 to 80754d1 Compare April 6, 2021 09:23
@v1v
Copy link
Contributor Author

v1v commented Apr 6, 2021

I've just changed the implementation to use the suggestion from #2336 (review)

Though, I might need some help with the test data

@sileht
Copy link
Member

sileht commented Apr 6, 2021

Fixtures for the functional need to setup a GitHub app and a couple of GitHub accounts and API keys.

I will take care of it.

@sileht sileht changed the title feat: add assign_author option to the copy and backport commands feat: add author option to the copy and backport commands Apr 7, 2021
@sileht sileht changed the title feat: add author option to the copy and backport commands feat: add assignees option to the copy and backport commands Apr 7, 2021
Copy link
Member

@sileht sileht left a comment

Choose a reason for hiding this comment

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

I just post some remarks, but I have already fixed them locally. You don't have to fix them.

I'm currently generating the fixtures.

mergify_engine/actions/__init__.py Show resolved Hide resolved
@@ -349,4 +351,10 @@ async def duplicate(
json={"labels": effective_labels},
)

if assignees is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Need also checks len(assignes) > 0 to avoid useless HTTP request

Comment on lines 66 to 72
f.write(
f"""<head>
<meta http-equiv="refresh" content="0; URL={url}">
<link rel="canonical" href="{url}">
</head>
""")
"""
)
Copy link
Member

Choose a reason for hiding this comment

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

I will fix the tox target

@sileht sileht force-pushed the feature/add-assignee-author-backport branch from 80754d1 to 90bf0c2 Compare April 7, 2021 09:43
@sileht sileht requested a review from jd April 7, 2021 09:43
sileht
sileht previously approved these changes Apr 7, 2021
@mergify mergify bot dismissed sileht’s stale review April 7, 2021 09:43

Pull request has been modified.

@mergify mergify bot requested a review from a team April 7, 2021 09:47
jd
jd previously requested changes Apr 8, 2021
Copy link
Member

@jd jd left a comment

Choose a reason for hiding this comment

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

Just small doc change

Comment on lines 76 to 79
- The users to assign to the pull request.

The list of users in ``assignees`` is based on :ref:`data type template`, you can use
e.g. ``{{author}}`` to assign the pull request to its author.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- The users to assign to the pull request.
The list of users in ``assignees`` is based on :ref:`data type template`, you can use
e.g. ``{{author}}`` to assign the pull request to its author.
- Users to assign the newly created pull request. As the type is :ref:`data type template`, you could use, e.g., ``{{author}}`` to assign the pull request to its original author.

@@ -94,5 +100,6 @@ added and the pull request merged:
backport:
branches:
- stable
assignees: ["{{ author }}"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assignees: ["{{ author }}"]
assignees:
- "{{ author }}"

I feel it's more readable :)

Comment on lines 54 to 60
* - ``assignees``
- list of :ref:`data type template`
-
- The users to assign to the pull request.

The list of users in ``assignees`` is based on :ref:`data type template`, you can use
e.g. ``{{author}}`` to assign the pull request to its author.
Copy link
Member

Choose a reason for hiding this comment

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

Change like backport above.

@sileht sileht force-pushed the feature/add-assignee-author-backport branch from 90bf0c2 to 791fd5c Compare April 8, 2021 08:56
@mergify mergify bot dismissed jd’s stale review April 8, 2021 08:57

Pull request has been modified.

@mergify mergify bot requested a review from a team April 8, 2021 09:00
@mergify mergify bot merged commit 16ac33c into Mergifyio:master Apr 8, 2021
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.

Assign the original author to the backported PRs
3 participants