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

Bevy Development Process #2256

Closed
3 of 4 tasks
NathanSWard opened this issue May 25, 2021 · 72 comments
Closed
3 of 4 tasks

Bevy Development Process #2256

NathanSWard opened this issue May 25, 2021 · 72 comments
Assignees
Labels
A-Meta About the project itself C-Feature A new feature, making something new possible

Comments

@NathanSWard
Copy link
Contributor

NathanSWard commented May 25, 2021

Rendered

What problem does this solve or what need does it fill?

A clearer defined set of rules and practices of project management issues for the bevy development process.

Since bevy is exponentially growing and more and more contributors are entering, we need a better defined practice for managing and organizing issues and pull requests.

What solution would you like?

Introduction of a proper bevy project board inside github, with automated movement between columns based on reviews/approvals etc.
Revamp of github labels.
A triage-team or something of the sort for those in change of assigning proper labels.

What alternative(s) have you considered?

Relying on the current tag/labeling scheme.

  • This however is not the best, since there is not a clear/defined structure for adding labels and how to organize the issues/prs that come in.

Using external kanban board software to track issues.

  • Relying on external software should be avoided, especially if Github provides the necessary features.

A few helpful resources

Github project board automation

TODO

@NathanSWard NathanSWard added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled help wanted and removed S-Needs-Triage This issue needs to be labelled labels May 25, 2021
@alice-i-cecile alice-i-cecile added A-Meta About the project itself and removed help wanted labels May 25, 2021
@NathanSWard
Copy link
Contributor Author

NathanSWard commented May 26, 2021

I'm still a fan of adding the appropriate type emoji on a PR title, if this is something we want to pursue.
It makes it simple when viewing commits (as the title becomes the first commit line) and being able to tell at a glance what a commit did without reading into it. ☺️☺️

At least I would want to add them to my PRs.....

@alice-i-cecile
Copy link
Member

Can you test what it does to relevant links in a test repo @NathanSWard? That's my largest hesitation left on the gitmoji front.

@NathanSWard
Copy link
Contributor Author

Can you test what it does to relevant links in a test repo @NathanSWard? That's my largest hesitation left on the gitmoji front.

By relevant links, do you mean when they live in the labels?
Since my current proposal would be to remove them from labels, but add them to the PR titles.

@alice-i-cecile
Copy link
Member

Oh wait, the PR title isn't part of the PR links. Okay, we're good then.

@mockersf
Copy link
Member

On the lifecycles, there are no transition to reject / close issues or PRs.

Labels should be prefixed by something indicating their kind: T-Enhancement, C-Audio, P-Web, .... This helps identify them when reading, and also help when adding/editing a pr/issue labels as they will be sorted.

About reviews:

Once in the Reviewer Approved column, the PR will be officially reviewed by Cart.

I think it's more "the PR will me marked as S-ready-for-official-review(or S-ready-for-cart while Cart is the only final reviewer, but I would love to move away from naming people in labels). Cart reviews are (currently at least) not direct.

About emojis in PR title: while I like having emojis in the git log, it feels strange having it both in the label and in the PR title. If they differ, what happens?

@NathanSWard
Copy link
Contributor Author

On the lifecycles, there are no transition to reject / close issues or PRs.

Hm yes this is true. There initially was however since we use bors, the original PR is marked as closed so there is no way to distinguish between a PR closed by bors and a PR closed because of another reason.
A possible solution to this is adding an closed status label which we can put on these PRs?

Labels should be prefixed by something indicating their kind: T-Enhancement, C-Audio, P-Web, .... This helps identify them when reading, and also help when adding/editing a pr/issue labels as they will be sorted.

Yep I like this a lot!

I think it's more "the PR will me marked as S-ready-for-official-review(or S-ready-for-cart while Cart is the only final reviewer, but I would love to move away from naming people in labels). Cart reviews are (currently at least) not direct.

Agreed! I think having a specific label for ready-for-official-review would be greatly beneficial and would help organize the PRs.

About emojis in PR title: while I like having emojis in the git log, it feels strange having it both in the label and in the PR title. If they differ, what happens?

So my solution would be to only have the emojis in the PR title and not in the label attached to it.

@mockersf
Copy link
Member

On the lifecycles, there are no transition to reject / close issues or PRs.

Hm yes this is true. There initially was however since we use bors, the original PR is marked as closed so there is no way to distinguish between a PR closed by bors and a PR closed because of another reason.
A possible solution to this is adding an closed status label which we can put on these PRs?

PRs merged by bors are prefixed with [Merged by Bors], can you catch that?

@NathanSWard
Copy link
Contributor Author

PRs merged by bors are prefixed with [Merged by Bors], can you catch that?

Not with github automation afaik :(.
However, I just updated the PR labels to include a S-Closed for when we close a PR without merging it.

Also, just addresses the other concerns and edited the write up :)

@NathanSWard
Copy link
Contributor Author

@cart
I would love any additional comments you may have about the current layout :)

@cart
Copy link
Member

cart commented May 27, 2021

I think we're definitely getting closer now!

However, I just updated the PR labels to include a S-Closed for when we close a PR without merging it.

This only works if we remember to add the tag. I want to avoid "extra process" and that definitely feels like it. Additionally, every time someone closes their own pr, now a member of the triage team needs to remember to throw the label on or our entire dataset is compromised.

The triage team will [...] as well as updating the PR title to insert content-label.

I dislike this. It is redundant work on top of labels and it isn't easily queryable / structured. Additionally, I think "bug fix" vs "enhancement" is arbitrary enough that it doesn't deserve to be front-and-center in the commit log. Some bug fixes are major enhancements. Some enhancements are also bugfixes. Crude "feature" vs "fix" categories could detract from the messaging / impact of a PR. I think they're still valuable as labels, I just don't think they deserve front-and-center attention.

emojis in labels / color as category

I'm not a huge fan here either. Compare this bug label from Vue's repo:
image

To this bug label from Bevy:
image

The image ends up being so small that it stops being legible as a bug. I personally think color is more effective at creating associations. red==bad / bug basically everywhere (yes color perception isn't universal across cultures, but thats also true for emoji perception).

I know this throws a wrench in the "color as category" design. But I think its more useful to use color to assist in distinguishing between "bug" and "enhancement" than it is to distinguish between "bug" and "platform".

I do think I like the prefixes for category ( [T-], [C-], etc), but I personally think thats enough of a distinguisher. Using color too is redundant information.

Content [C-]

This feels more like "category" than "content".

TODO: with the priority label, I’m still not sure if we want to just tag the critical/high priority items, or to have tags for all (e.g. P0, P1, P2, P3).

I'm still very much in favor of only labeling high priority items. "purity" isn't a good enough reason to introduce a bunch of noise into the system imo. Something like 95% of issues will be "normal" priority. And using "critical" / "high" priority gives immediate context relative to, say, P1 and P2 (because users won't know how many priorities there are / what the spectrum is).

@Weibye
Copy link
Contributor

Weibye commented May 28, 2021

Should there be a "Needs RFC" status tag, or is that already covered by "Needs Design"?

@NathanSWard
Copy link
Contributor Author

NathanSWard commented May 28, 2021

This only works if we remember to add the tag. I want to avoid "extra process" and that definitely feels like it.

Yep that's fair. I was mostly looking for a way to distinguish closed/merged PRs, however with bors it makes this a little difficult. So leaving it off is probably best.

I dislike this. It is redundant work on top of labels and it isn't easily queryable / structured. Additionally, I think "bug fix" vs "enhancement" is arbitrary enough that it doesn't deserve to be front-and-center in the commit log.

This was referring to the content labels which is (ECS, Assets, Rendering, etc...)
I do agree that it's extra work and maybe not warranted, but being able to categorize commits by just their first line is very helpful. However I've seen to have gotten quite a but of pushback from both this and the gitmoji, and I'm not longer super in favor of them, so these I'll remove.

The image ends up being so small that it stops being legible as a bug.

Yep I also agree! I actually removed these from the hackmd 😉

I do think I like the prefixes for category ( [T-], [C-], etc), but I personally think thats enough of a distinguisher. Using color too is redundant information.

Im definitely on the side of both using the prefix T- and colors. The prefix makes it easier to search for a specific label, and also keeps the sorted alphabetically. However I would also argue the colors should stay consistent between label categories, as this helps with visual organization. Granted, while i'm on this side, I'm not against removing the color scheme, since the prefix approach is sufficient at keeping the organized. I'll probably defer to @alice-i-cecile on this, since she recommended to me the color schemes.

I'm still very much in favor of only labeling high priority items.

I don't feel strongly either way about this, however I do like how only labeling the high priority targets removes a lot of the noise :)
So I'm happy with only having like critical and high priority labels :)

@alice-i-cecile
Copy link
Member

Re: color, I worry that we simply have too many labels to meaningfully use color to describe the content (rather than category) of the label. I suppose we could assign colors to labels at random and then make use of them to distinguish filtered search results?

Using colors for the category helps with labelling, and guides the eye when you're trying to check for the type of information you care about.

Should there be a "Needs RFC" status tag, or is that already covered by "Needs Design"?

IMO this is covered by "needs design" + "complex".

This feels more like "category" than "content".

Totally fine by me. I just wanted a different first-letter and "content" was the first word that sprung to mind.

@cart
Copy link
Member

cart commented May 29, 2021

A triage-team or something of the sort for those in change of assigning proper labels.
Create a triage team?

Haha are you aware that we already have a triage team (and that you are a member)?
https://github.com/orgs/bevyengine/teams/triage-team

@cart
Copy link
Member

cart commented May 29, 2021

IMO this is covered by "needs design" + "complex".

agreed. although that might not be immediately apparent to some people.

Yep that's fair. I was mostly looking for a way to distinguish closed/merged PRs, however with bors it makes this a little difficult. So leaving it off is probably best.

I really hate that we need to choose between "ideal commit history" and "ideal github states / statistics". Hopefully some saint will find a way to solve this for us: bors-ng/bors-ng#1217

However I've seen to have gotten quite a but of pushback from both this and the gitmoji, and I'm not longer super in favor of them, so these I'll remove.

Thanks :)

Using colors for the category helps with labelling, and guides the eye when you're trying to check for the type of information you care about.

Colors are fortunately the easiest thing to change (because we can do it "globally" with a single click). I'm cool with experimenting here. You both like "colors for categories" so I'm cool with trying that first (although I do think throwing away red for bugs is a major loss given how common that is / how strong those associations are).

@alice-i-cecile
Copy link
Member

although that might not be immediately apparent to some people.

Yeah. That said, I expect that we should basically never be dropping the needs-rfc label on others work without any explicit discussion about why (and links to the RFC process for new contributors).

@cart
Copy link
Member

cart commented May 29, 2021

Good point!

@NathanSWard
Copy link
Contributor Author

As was brought up in #2277 , @alice-i-cecile should we have a content label per crate inside crates/?

@mockersf
Copy link
Member

As was brought up in #2277 , @alice-i-cecile should we have a content label per crate inside crates/?

Not Alice, but I think so!
Would it be possible to add labels automatically based on files changed?

@alice-i-cecile
Copy link
Member

I'd love this if it was automatic.

@NathanSWard
Copy link
Contributor Author

Would it be possible to add labels automatically based on files changed?

I'll take a look into this and see if it's possible :)

@mockersf
Copy link
Member

I prefer 05 without gradient 👍

  • gradient makes it look like some are less important than others, the darker stands out less than the lighter
  • gradient will be painful in the future when we want to add a new item to a category and have to check where we stopped, update the gradient, redo all the category if we're getting too close to white/black, ...

@Weibye
Copy link
Contributor

Weibye commented Jun 11, 2021

Scheme 06

This is a non-gradient version of scheme 05.

This is the colors for each group, with exceptions for the labels that keep their existing color and does not match with the rest of the group.
image

Existing colors

Label Status
C-Bug Keeps its color and becomes outlier in group
C-Crash Keeps its color and becomes outlier in group
C-Invalid Keeps its color and becomes outlier in group
E-Help-Wanted Keeps its existing color and the color is adopted to the whole group
S-Needs-Investigation Keeps its existing color and the color is adopted to the whole group
C-Docs Gets new color from the group
E-Good-First-Issue Gets new color from the group
S-Wontfix Gets new color from the group
C-Duplicate Gets new color from the group
Expand to view full scheme

image

Thoughts

  • Do C-Crash and C-Bug need to have different colors or should they simply be the same?
  • Is there significant value in keeping S-Invalid as an outlier? (I have very little experience with some of these labels)
  • Are there other labels that should be outliers, that aren't?

Note: Again, any issues related to readability of the colors should be a non-issue as github label system picks a foreground-background color pair that ensures readability

@Weibye
Copy link
Contributor

Weibye commented Jun 11, 2021

This is what it would look like as labels (using scheme 06)

image

@NathanSWard
Copy link
Contributor Author

Scheme 06

Yep! This looks great! :)
The only thing I would add is that C-Hotfix should have its own color (since hotfixes are usually pretty crucial)

@TheRawMeatball
Copy link
Member

We should probably have a uses-unsafe label for PR s

@Weibye
Copy link
Contributor

Weibye commented Jun 15, 2021

What group / category would Uses-unsafe belong under? Category?

@MinerSebas
Copy link
Contributor

As mentioned in #2436, I think we also need a Needs Migration Guide Label.

@cart
Copy link
Member

cart commented Jul 9, 2021

I agree (and just created that label ... we can align label naming with the conventions we decide on here)

@cart
Copy link
Member

cart commented Jul 9, 2021

Sorry for the delay. Scheme 6 looks reasonable to me. I'm gonna make this the "last call" message so we can move forward.

Leave a 👍 if you agree with adopting Scheme 6
Leave a 👎 if you disagree (and explain why).

I'll give this a few days to collect responses. If we have relative consensus I'll make the switch.

@mirkoRainer
Copy link
Contributor

fwiw, as a colorblind user. I appreciate no gradients and text prefix labeling.

@cart
Copy link
Member

cart commented Jul 10, 2021

fwiw, as a colorblind user. I appreciate no gradients and text prefix labeling.

Thanks for calling this out. This was (embarrassingly) not on my radar.

@NathanSWard
Copy link
Contributor Author

NathanSWard commented Jul 10, 2021

Just as a note we'll need to update the PR-labeler action to use the new needs-triage label

@mirkoRainer
Copy link
Contributor

fwiw, as a colorblind user. I appreciate no gradients and text prefix labeling.

Thanks for calling this out. This was (embarrassingly) not on my radar.

No worries! Luckily most modern OS's have a toggle to make it easier to differentiate colors, but then everything is "off" color. 😵‍💫

Also, the text prefix/tags will help with any high-contrast users as well.

@cart
Copy link
Member

cart commented Jul 13, 2021

Alrighty I've done the label migration (including updating our templates and auto labelers)!

Changes I made to the current proposal that we can discuss

  • Changed C-Duplicate to S-Duplicate. Imo being a duplicate isn't an "issue category type". It is a status.
  • Added a C-Question category (and migrated "question" labels). Whether or not this should exist is up for debate. I think most (if not all) questions should be converted to Github Discussions. As a followup, I think we should do a pass over open issues with this tag, convert the ones that should be discussions, then decide if C-Question is still needed.
  • Added C-Usability (and migrated "usability" labels). This seems like a reasonable category and there were many existing issues with the "usability" label.
  • Renamed "C-CI-Build-System" to "C-Build-System" because I like the terseness of the latter name and the former is not fun to look at. I think CI fits cleanly into a generic "build system" category.
  • I didn't add the "C-Hotfix" label because we don't have a hotfix process in place yet (ex: we don't yet have a formal process for patch releases). We should come up with a process here and once we do, I'm happy to add the label. But until then its just noise.
  • I didn't add a "C-Code-Cleanup" label. I think "C-Code-Quality" encompasses code cleanup.
  • I didn't add "A-Ecosystem" and instead renamed "meta" to "A-Meta". "A-Ecosystem" feels more like a "third party dependency" category, which we already have. We use "A-Meta" feels like a good name for what we're using it for: "meta bevy project discussions".
  • Didn't add a "S-Invalid" label. The definition was "This is spam, a duplicate issue, or user error". But spam should be deleted, duplicate issues already have the S-Duplicate label, and user error already has the S-User-Error label. If we need more states, we can add them, but S-Invalid felt too generic and overlapped too much.

Next steps

  • Update our project board(s) to account for these new labels
  • Convert relevant "C-Question" issues to Github Discussions
  • Others should review the current set of labels and make sure everything looks good.

@mirkoRainer
Copy link
Contributor

mirkoRainer commented Jul 14, 2021

  • Convert relevant "C-Question" issues to Github Discussions

done & done

Example:
https://github.com/bevyengine/bevy/discussions/2465

@cart
Copy link
Member

cart commented Jul 14, 2021

As a heads up: we opted to roll with automated discussion migration (which maintainers with write access have access to). This preserves comments, reacts, etc.

@cart
Copy link
Member

cart commented Jul 14, 2021

I'm going to remove the C-Question label for now. Issues with questions (that don't end up being actionable issues) should be migrated. Just ping me with a link and I'll do it.

Ideally eventually github will give us more control over permissions so I can delegate this work, but currently the only way to delegate is to give other people full write access to the repo, which i don't want to do 😄

@cart
Copy link
Member

cart commented Jul 14, 2021

Just as a heads up, I'm not currently doing the work to set up the project boards. Triage Team members are welcome to collaborate on that.

@alice-i-cecile
Copy link
Member

Ideally eventually github will give us more control over permissions so I can delegate this work, but currently the only way to delegate is to give other people full write access to the repo, which i don't want to do 😄

Can @mockersf do this too? Seems to fit well under "uncontroversial cleanup work" .

@mockersf
Copy link
Member

I don't have write access, I have bors permissions... so I can only do what bors knows how to do.

@cart
Copy link
Member

cart commented Jul 14, 2021

Yup because repo write access has the ability to "rewrite history" and there aren't really ways to audit that, I'm pretty hesitant to delegate it even to the most trustworthy of people (and I consider @mockersf pretty trustworthy 😄 ). I'll revisit that if scalability of these types of actions is ever an issue, but for now I really like that bors exclusively exposes "append to history" operations.

@bjorn3
Copy link
Contributor

bjorn3 commented Jul 14, 2021

With branch protection I think only admins can rewrite history.

@cart
Copy link
Member

cart commented Jul 14, 2021

Ah yeah I think you're right. Even so, the Write role has way too much control over random things in the repo:

I'm still pretty biased to wait until scalability becomes an issue or github gets customizable RBAC. But knowing that write access can't rewrite protected branches definitely makes me more comfortable.

@alice-i-cecile
Copy link
Member

I think that Meta is actually an Area, not a Content. See this issue, where it's clearly an enhancement to some meta process, or the Contributing.md work, which is a docs improvement to a meta process.

@cart
Copy link
Member

cart commented Jul 16, 2021

I'm convinced! Just changed it.

@TheRawMeatball
Copy link
Member

Hmm, just leaving this so it's not lost: A couple days ago, there was a discussion on discord about what should be issues and what should be discussions.

Personally, I'd prefer to use discussions for feature requests and other nebulous enhancement ideas, while keeping issues for bug reports, tracking issues and immediately actionable stuff, as discussions can do basically everything issues can but better, can be easily converted to issues once they're actionable, and keep the open issue count low (though how important that is can be debated). A counter-point to this is discussions are still in beta, and not a part of most people's github workflows, though this could be fairly easily remedied by noting discussions are prefferred in the enhancements issue template.

ostwilkens pushed a commit to ostwilkens/bevy that referenced this issue Jul 27, 2021
This is a first step at addressing bevyengine#2256 via adding a pr template.
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Apr 25, 2022

Closing this out for now; I don't think that the remaining steps are quite the right direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Meta About the project itself C-Feature A new feature, making something new possible
Projects
None yet
Development

No branches or pull requests

9 participants