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

CIP PR queue overhaul, state vocabulary & tagging #883

Closed
rphair opened this issue Aug 19, 2024 · 10 comments
Closed

CIP PR queue overhaul, state vocabulary & tagging #883

rphair opened this issue Aug 19, 2024 · 10 comments

Comments

@rphair
Copy link
Collaborator

rphair commented Aug 19, 2024

As one of the CIP process major tasks we talked about last year, I'm starting to:

  1. go through the PR queue, starting with the oldest PR and going forward;
  2. closing abandoned proposals (assuming we've given suitable notification in advance);
  3. tagging the ones that remain, according to what we appear to be waiting for.

Currently there are 77 open CIP PRs in the queue (minus a couple that are "housekeeping") and it is time we closed off the ones quick are physically abandoned, relying on the submitter to reopen them & identify the pending issues if they wish to pursue that proposal.

As a result we will quickly arrive at a state tagging vocabulary that will tell us quickly which sets of PRs need a particular type of action for a proposal to progress (or for a PR to be closed, if that action remains undone with no reason given).

I've yet to start (and some more ideas may emerge for "intermediate" states by the time I get through the big list); here are the states I can imagine, including some we already have. I'll rename the existing tags so each state tag begins with State: and will ensure that all open, non-Draft CIP/CPS PRs have exactly one tag from the time they are first taken on by an editor:

  • State: Triaged - applied editor actions for a new proposal PR: qualifying a submission, adding readable link, category tagging, adding to agenda, (optional) quick format review
  • State: Unconfirmed - presented at CIP meeting, but not yet assigned a candidate CIP number (pending further review & update)
  • State: Confirmed - CIP number has been assigned (usually at CIP meeting)
  • State: Last Check (renamed from existing tag)
  • TBD: any other intermediate states?
  • State: Waiting for Author (renamed from existing tag)... any of these currently without CIP numbers should be tagged Unconfirmed
  • State: Abandoned ... applies to Candidate PRs (if not assigned CIP number yet, they're simply closed if inactive too long)
  • State: Likely Deprecated (renamed from existing tag) ... applies to Candidate PRs (if no CIP number yet, their closure doesn't "deprecate" anything)

This vocabulary will correspond to the existing Category: GitHub labels which also represent mutually exclusive categorisation matching the category in the CIP header. Though these categories were intended to recruit editors and attention from enlisted projects, the State: tags will be mainly for editors to keep the PR queue trim, readable & functional.

Please @Ryun1 @Crypto2099 be patient with me for the next day or so while I apply these tags... and definitely please suggest any others that come to mind to complete the above list. As soon as it's confirmed I'll post a simple outline which will serve a flow diagram, and then:

  • document it with a page on the new Wiki
  • update the README so it contains links to tag pages: showing proposals that are in the various stalled states (replacing than the manually written, subjective & error-prone Waiting for Author list).

@KtorZ this brings up some things I remember editors talking about from the very early days, including some artefacts from the long-discontinued CIP "state diagram" ... so if you have not moved on entirely from CIP editor duties I think we would all be happy to have your input as well.

@rphair
Copy link
Collaborator Author

rphair commented Aug 20, 2024

At this time I've gotten through almost of all the PR queue (from the really old, stalled PRs up toward the present time) but realised with the currently open ones tagged Update that we have a potentially different set of State: tags for CIP updates than we do for new CIP submissions.

Therefore I need to rethink some of this (over the next day) to be sure we don't end up with a huge number of tags: as a cross product of PR types -times- review states. The goal is a minimum number of states covering new PRs and CIP updates that still allows easily generating a report of candidate CIP / CPS PRs that have not been merged yet (to replace the manually generated tables on our front page README).

This PR is on the agenda for the CIP meeting tomorrow & quickly we might brainstorm about it then & in the meantime @Ryun1 @Crypto2099 you could look at the PR tagging on the older CIPs so far.

@Ryun1
Copy link
Collaborator

Ryun1 commented Aug 20, 2024

Again @rphair you have moved to improve the process for all
Thank you, this is really appreciated

A few comments from me;

  • The statuses are a reasonable start, happy to go with these and see what works and what doesnt, so we can improve
  • Recently on GovTool I added a couple github actions (actions here/here, added via 1718) which add labels to issues
    • I wonder if we could/should add similar workflows for pull requests
    • This could reduce the burden of maintaining labelling across PRs
    • Im happy to do this

@rphair
Copy link
Collaborator Author

rphair commented Aug 20, 2024

@Ryun1 that sounds great. About the timetable, I suggest doing these first before adding any automation:

  1. ensure we have a vocabulary of singly applicable tags that apply to 100% of PRs that aren't "housekeeping" or "DB update" (the white labels) - will be done in a day or so
  2. update & merge this period's README change to put the automatically generated CIP / CPS candidate & "Waiting" lists on the front page - will be done soon after
  3. apply the recently added Abandoned labels to reduce the total length of the PR queue, some reasonable time (1 month from now?) after holiday returns & projects resuming. which would close around 10 more old PRs and get the queue down within a size of 50... where it was 3 years ago and in my opinion providing a human readable (2 page) view, which will help clarify the effects of that automation.

@rphair
Copy link
Collaborator Author

rphair commented Aug 21, 2024

I've worked out the set of State: tags: currently the 7 above and considering an 8th if there is a popular demand for Waiting for Project as in the hypothetical requirement above... so far neither author nor Plutus team has asked CIPs to be put on hold in this way so the Waiting for Author could suffice.

We cannot really have a Candidate tag (I've changed it above to Confirmed) because it couldn't be applicable to those PRs tagged Update progressing in the review process without causing these updates to also appear on the list of newly submitted CIPs under review. So the working definition of a Candidate (to document also in the Wiki + README) is any open PR:

  • NOT a Draft
  • AND having any of these State: tags:
    • Confirmed
    • Waiting ... {for Author}
    • Likely ... {Abandoned, Deprecated}
    • Last Check
  • AND having none of these tags:
    • Biweekly / Housekeeping
    • DB updates (CIP-0010, etc.)
    • Update
    • Correction
    • Translation

This will allow, for instance, PRs tagged Update or Translation to be given the regular state tags that reflect their progress through the review process alone.

  • Trivial updates (Correction) and DB updates or Housekeeping (the "white" tags) should never be given State: tags but they're filtered out in determining the Candidate list as a failsafe to avoid these administrative PRs becoming accidentally linked with current, high-profile CIP candidates from the repository front page if a state tag is inadvertently applied. 😜

Sound good? Probably sounds too complicated but all the complexity will go into 1 single long GitHub label query string, and therefore the rest of the tagging remains simple: both for editors to apply and for readers to understand.

  • I'm quickly learning Mermaid which is currently building up the State: flow into a nice state diagram / flowchart that will make the CIP states quickly understandable by anyone.

@rphair
Copy link
Collaborator Author

rphair commented Aug 22, 2024

@Ryun1 @Crypto2099 the last thing I want to add ... before closing when I

  • publish the biweely README update, to replace the manually edited PR tables by tagging state queries (i.e. just regulary "merges" from now on + occasional deprecations from the merged CIPs)
  • link to a wiki showing the state diagram of the evolved 7 (so far) states

... will be that I think the first (Triage) and last (Last Check) CIP meeting agendas should be replaced also by state queries. See meeting # 96 agenda & let me know what you think: https://hackmd.io/@cip-editors/96

This will remove the most common source of error in meeting agenda building... us missing something... and all the shuffling around we have to do in editing the HackMD agenda file by reconstructing the links to the GitHub PR pages.

  • With a single link in each section these 2 parts of the agenda will always be built automatically, leaving us to choose which ones to Review (a section always built manually) at editor discretion based on GitHub progress and/or author/community request for escalation.

So from now on, nothing close to the beginning or end of the CIP process will get missed... as long as we can do what we should now commit to doing: every PR that isn't "housekeeping" or "database update" (i.e., all the "white" tags) should have exactly one State: tag.

p.s. TESTING label formatting below (needs to be on same repository): State: Triage Applied to new PR afer editor cleanup on GitHub, pending CIP meeting introduction.

@KtorZ
Copy link
Member

KtorZ commented Aug 23, 2024

Went through the issue and, my two lovelaces:

  • tags are good, but too many tags and it becomes hard to follow and comprehend. I usually find tags good to capture topics and classify items, but cumbersome to track states.

  • rather than tags, it seems to me that you're looking for a trello-like card lane; where items can move from one column to another. We could easily set that up with GitHub; and add some degree of automation to it so that it plays nicely with tagging and events such as opening / closing PRs.

  • we could also entertain the idea of introducing a CIP bot, that would automatically mark inactive PRs, send a "warning" to the author and close it afterwards if no activity is detected. If there's no active discussion on a PR and it's still hasn't been merged, there's no point keeping a PR open. It remains accessible when closed. Hence I don't think we should be particularly "gentle" on the PR closing. It's just housekeeping and if it hurts people's feeling then I don't know what to say... grow up folks 😅.

@rphair
Copy link
Collaborator Author

rphair commented Aug 23, 2024

thanks @KtorZ - at this stage we can authenticate the 7 states which I think is about the maximum people can differentiate (I'm posting a state diagram on the Wiki today; even with 7 states it's dense), and over the next month use the abandonments to reduce the queue to a human readable size again & then look at more automation improvements.

There is an exasperating side discussion in this Forum thread, beginning here: https://forum.cardano.org/t/ecosystem-maps-do-we-need-a-set-of-standards-for-defining-ecosystem-roles-relationships-and-sectors/135155/6

... which I think emphasises the importance of keeping CIP process implementations simple enough for casual observers to understand them. I would want to pursue your suggestion, but depending on how it appears to the public vs. a more "folksy" vocabulary of labels, it could be an argument for the latter. 🤔

@rphair
Copy link
Collaborator Author

rphair commented Aug 23, 2024

OK @Ryun1 @KtorZ I have the whole state tagging process described here on the Wiki, including a state diagram of the 7 states and how this can streamline editor workflow with only native GitHub features & without any additional software for now: https://github.com/cardano-foundation/CIPs/wiki/301.-State-tagging

It turns out after all that fluff in the queue we only had 9 candidates. The problem I suppose was that we didn't rigorously apply the states or use them for querying what could be escalated, re-reviewed, or closed. The last complete review has targeted dozens of likely abandoned PRs we can close in the near future (maybe when sure everybody is back from "summer vacation").

Now I expect we will be staying ahead of it: with maybe some future improvements like sorting the Waiting for Author & Likely Abandoned lists by date of last comment (and adding that query to the Reporting section so we can apply it periodically).

@rphair
Copy link
Collaborator Author

rphair commented Aug 24, 2024

Completed by #887.

@rphair rphair closed this as completed Aug 24, 2024
@rphair
Copy link
Collaborator Author

rphair commented Aug 24, 2024

p.s. re: #883 (comment) one of the Catalyst reviewers for this task confirmed that a "card lane" would be beneficial, which I guess confirms this would be well understood by observers. So @KtorZ I'll put on my TODO to adapt this state model into a card lane around end of year & if you have any suggestions please feel free to send them on.

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

No branches or pull requests

3 participants