Skip to content

Instantly share code, notes, and snippets.

Show Gist options
  • Save marcoceppi/cc516978945e75a0f722 to your computer and use it in GitHub Desktop.
Save marcoceppi/cc516978945e75a0f722 to your computer and use it in GitHub Desktop.

Hi everyone, I'm going to jump right in with this email be briefly describing a short summary of the issues people have brought up around the review process and the review queue, then outline what my proposal to fix this is, outline the new process for feedback prior to implementation, finally listing actions left to implement this.

Issues

One of the biggest feedback we've gotten from users lays along the lines of "where is my charm in the review process and what are next actions". Currently, the review queue doesn't illuminate any next actions for a user. As queue times increase this can leave a poor user experience. Another feedback point we have seen recently are reviews being lost in the process. This is arguably the primary reason behind a revamp of the review queue and review process to ensure a submitted item for review does not get lost in the process. These are two primary examples, while a more exhaustive list exists these items have the most impact for users contributing to the ecosystem.

Solution in Development

For the past two weeks I've been working on a replacement review queue which uses a lot of the initial code from the current review queue. A few major points:

  • Ingests all bugs and merges regardless of state, owner, or series
  • Tracks reviews once ingested
  • Decoupled from charmworld
  • API access
  • Detailed statistic gathering
  • Track reviews per user
  • Track merges from LaunchPad and Github
  • Priority sorting of reviews

This is what has been done for the first version of the review queue which I plan to release this week for review. However, as the review queue is currently tracking ALL items (and not items for which only a charmer can take action against) we will need to update the review process for charmers and charm-contributors (Jr. Charmers) alike. Those proposed changes are outlined below:

Proposed review policy changes

New Charms

With the new review queue, charmers being assigned to the bug is no longer a requirement for it to appear in the queue. Any bug against the charms distribution which isn't targeted at a source package (charm) will be ingested. Currently, "NEW" and "FIX COMMITTED" charms appear in the queue. Going forward, LP statuses will have more effect on the queue. "NEW" status will appear in a separate queue. These are charms that are just being submitted. The new queue is designed to get first contact to that user faster. "TRIAGED" should be what a reviewer (who isn't a charmer) moves the bug status to if it passes an initial review "FIX COMMITTED" will indicate that the charm author has received review items and is ready for another review, "INCOMPLETE" or "IN PROGRESS" indicates that someone has reviewed the charm and it needs work to complete. "FIX RELEASED" when the charm has been promulgated. All other statuses indicate an abandoned/closed review.

Proposed workflow

  1. User submits a new charm by opening a bug and linking a branch to that bug
  2. It appears in the incoming review queue, any person (charmer, charm-contributor) provides initial review (Welcome, thanks for submitting, proof, etc) and outlines the process for the user by either linking to the documentation or use of boilerplate text. If the charm passes initial review, reviewer moves bug to TRIAGED, if it fails, moves bug to INCOMPLETE. Author will need to move bug to "FIX COMMITTED" once issues are resolved.
  3. Once bug is in TRIAGED, a ~charmer will review the submission. If this passes review charmer promulgates and moves bug to "FIX RELEASED", otherwise places the bug in "INCOMPLETED".
  4. If further action is needed, once author resolves, moves bug to "FIX COMMITTED" to have it re-reviewed.

Updates to charms

When reviewing merges to a charm, the merge proposal no longer needs to be assigned to charmers to show in the queue, any merge proposal against a promulgated charm branch will appear in review. Likewise, voting on a merge proposal will not directly remove it from the queue! So feel free to vote in the affirmative/negative often when reviewing merges. If a merge needs work you will need to move the merge status from "NEEDS REVIEW" to "WORK IN PROGRESS" which will remove it from the queue. The author then needs to move the review back to "NEEDS REVIEW" for it to re-enter the queue.

Actions

  • Seek feedback and approval of updated process
  • Release first version of review queue
  • Clean up outstanding (no longer valid) items
  • Update documentation to reflect new review process

I'm interested in concerns, feedback, and suggestions as I continue to work on an improved charm experience.

Thanks, Marco Ceppi

@arosales
Copy link

Marco,
Thanks for taking some time to write this up and improve the review process.

+1 on "where is my charm in the review process and what are next actions" being a large pain point from users. I have received this feedback too.

On the section "## New Charms" it would be helpful to know who has the responsibility to update the bug status, and what direct action that has on the review queue, specifically where that places the charm in the review queue. To that note a high level summary of how a charm review life cycle would work in your proposal would also be helpful. Specifically, for the new charm suggest to also be very blunt about what action is needed by a submitter to "submit a new charm."

Suggest to also separate out info/actions for reviewers and submitters in the workflow.

As an ask since you are tracking reviews it would be very helpful to know say on a given timeline (weekly, monthly) what reviews were done so folks can see their changes being done, and other folks can see what changes are being made to the ecosystem.

@marcoceppi
Copy link
Author

Updated

@arosales
Copy link

New Charms

  1. User submits a new charm by opening a bug and linking a branch to that bug

Where does the user log the bug?

  1. It appears in the incoming review queue, any person (charmer, charm-contributor) provides initial review (Welcome, thanks for submitting, proof, etc) and outlines the process for the user by either linking to the documentation or use of boilerplate text. If the charm passes initial review, reviewer moves bug to TRIAGED, if it fails, moves bug to INCOMPLETE. Author will need to move bug to "FIX COMMITTED" once issues are resolved.

To confirm the bug is in a "New" state and in a specific "incoming review queue." Also what are the other queues?

Updates to charms

On "Updates to charms" what is the workflow if there is a bug involved with the merge? Is is just linked in the merge request and no specific action is taken?

If a merge needs work you will need to move the merge status from "NEEDS REVIEW" to "WORK IN PROGRESS" which will remove it from the queue.

Is there any way to keep this tracked but shown in the review queue under a different state? The idea being that other interested folks and still keep track of this review as being in progress. Also is there a "triaged" state for "Updates to charms?" It seems it is either nacked (Work in Progress} or promulgated. Any reason "updates" to charms should follow the same workflow as "New Charms?"

General

Does this review system easily translate if we move charms away from LP and directly to the charm store?

-thanks,
Antonio

@marcoceppi
Copy link
Author

  1. Launchpad, I'll clarify
  2. There's queues for each type of work, Incoming charms, main review queue, Ask Ubuntu questions, Tool merges (charm-tools, amulet, charmhelpers, etc)
  3. User can link a bug if they wish, there's no restriction or guidance here.
  4. This is a limitation of Launchpad, there is no triaged state in the merge request interface.
  5. Yes, it's plugin oriented. As long as new charm store has an API we can build around it.

@arosales
Copy link

  1. Launchpad, I'll clarify

Thanks.

  1. There's queues for each type of work, Incoming charms, main review queue, Ask Ubuntu questions, Tool merges (charm-tools, amulet, charmhelpers, etc)

This would also be helpful to clearify.

  1. User can link a bug if they wish, there's no restriction or guidance here.

Ack, also perhaps good to clearify on the docs page if this policy get +1'ed.

  1. This is a limitation of Launchpad, there is no triaged state in the merge request interface.

Understood.

  1. Yes, it's plugin oriented. As long as new charm store has an API we can build around it.

Thanks for confirming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment