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

Extended action #75

Merged
merged 3 commits into from
Jul 8, 2022
Merged

Extended action #75

merged 3 commits into from
Jul 8, 2022

Conversation

Mossop
Copy link
Member

@Mossop Mossop commented Jul 5, 2022

Adds a new action that adds support for syncing the assignee and status of bugs over to Jira. The assignee is fairly blind, if the bug assignee exists as a Jira user who can be assigned to the ticket then it does so. The status is more complex as different projects have different statuses so this lets each whiteboard tag define a mapping from the bugzilla status to the jira status. In both cases we only attempt to update jira if the bug field actually changed (or if it is a new bug).

I'm a bit unsure over whether this should be a new action or just an addition to the default action. Certainly the status syncing can be disabled by simply not providing a map. Conceivably the assignee syncing could be made to be configurable. I know we want this in at least two projects, probably more. That also made me fail to come up with a decent name for the action!

@Mossop Mossop requested a review from a team as a code owner July 5, 2022 14:44
Copy link
Contributor

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

Thanks! This is cool!

After a quick overlook, before going into deeper review, could you please:

  • add some logging (debug) especially for situations where the action gives up (using struct logs like in Improve logging using context (fixes #73) #74)
  • pass kwargs explicitly (eg. payload.map_as_comments(status_log_enabled=False, assignee_log_enabled=False) instead payload.map_as_comments(False, False))
  • Add a paragraph of documentation in the README about this action

@Mossop
Copy link
Member Author

Mossop commented Jul 6, 2022

@leplatrem Updated.

Copy link
Contributor

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

This looks good!

I noted a few nits, feel free to ignore them if you don't find them relevant.

Regarding the naming, extended is fine. Alternatives could be:

  • an explicit naming like default_with_assignee_and_status
  • additional parameters on the default action
  • a refactor to execute multiple atomic actions instead of a single one

We can discuss this in a dedicated issue. We are learning on the way here :)

Note:
I started a big refactor of tests in #76 . But I'll merge this PR first and rebase the refactors on top of it

src/jbi/bugzilla.py Show resolved Hide resolved
src/jbi/whiteboard_actions/default.py Outdated Show resolved Hide resolved
src/jbi/whiteboard_actions/extended.py Outdated Show resolved Hide resolved
src/jbi/whiteboard_actions/extended.py Outdated Show resolved Hide resolved
src/jbi/whiteboard_actions/extended.py Outdated Show resolved Hide resolved
src/jbi/whiteboard_actions/extended.py Outdated Show resolved Hide resolved
tests/unit/jbi/whiteboard_actions/test_extended.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tests/unit/jbi/test_bugzilla.py Outdated Show resolved Hide resolved
tests/unit/jbi/whiteboard_actions/test_extended.py Outdated Show resolved Hide resolved
@Mossop
Copy link
Member Author

Mossop commented Jul 7, 2022

This looks good!

I noted a few nits, feel free to ignore them if you don't find them relevant.

Regarding the naming, extended is fine. Alternatives could be:

* an explicit naming like `default_with_assignee_and_status`

* additional parameters on the default action

* a refactor to execute multiple atomic actions instead of a single one

I went with the different action since that was recommended to me as the thing to do for new behaviour. I actually think that syncing the status and assignee are likely to be such common needs (I've only talked to two teams using JBI but both of them want it) that unless you disagree I think we should just fold this into the default action. The status_map is already optional and I could just add an option for syncing the assignee defaulting to false so no-one is impacted at the start.

@leplatrem
Copy link
Contributor

unless you disagree I think we should just fold this into the default action.

I think it makes sense to fold it in another iteration, especially if all use-cases use it :)

We can also discuss this in #77 (thanks for your input and comment there!)

…assignee and status.

Separates out a couple of key parts of the default action to allow
sub-classes to make minor changes before and after Jira is updated. This
includes:

  * Allowing sub-classes to modify the comments generated for existing
    issues
  * Allowing sub-classes to make changes to the Jira issue after the
    default action is done processing.

Then defines a new action that extends the default and syncs assignee
and status.
@Mossop
Copy link
Member Author

Mossop commented Jul 8, 2022

@leplatrem I think I've addressed all the comments and also renamed the action to your suggestion so its purpose is clearer.

@leplatrem leplatrem merged commit a9f4915 into mozilla:main Jul 8, 2022
@Mossop Mossop deleted the extended-action branch July 8, 2022 10:22
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.

None yet

2 participants