-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Update to lists for 1st of December #7053
Conversation
@@ -1,13 +1,585 @@ | |||
# New | |||
ofl/afacad # https://github.com/google/fonts/pull/6881 | |||
ofl/kalnia # https://github.com/google/fonts/pull/6879 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kalnia still needs to be listed in the to_production
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is down the list in the description/metadata section, meaning that there might be a double tracking for it. It would be better to keep only the PR labeled as new font to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kalnia PR #6879 is in the traffic jam going to production, is this a problem with gftools
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talked to @m4rc1e about this and he says it is not an issue and the #6879 PR will be included.
The only thing that matters is that is references ofl/kalnia
, the PR is commented out.
It was not my choice to remove the #6879 PR in favor of the #7008 Metadata PR, that was gftools gen-push-lists
.
My understanding is that as long as the to_production
list points to ofl/kalnia
, everything in that directory will be pushed to production, and on the backend there is not system looking at the PR comments and only pushing files included in the PR comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eliheuer please don’t ignore my comment, this is about simple organisation when checking the fonts: as we said many times, it is more efficient to track only one PR and remove the double from the board to avoid confusion and keep it clean and readable, so the number of tracked PRs relates to the number of font dir path in the list. So we see quickly through the list how many new fonts are moving through the pipeline, which is an important information for Chris.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eliheuer This case is similar to the one we discussed in the chat before for PPS.
When preparing the lists, we must cross-reference the different PRs for a single project and then leave the one with the "new font" info as the more relevant to track it as a whole.
Otherwise, if done using the one labeled "Metadata" it could bring incomplete information about what to inspect in detail as part of the server's revision for the projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When preparing the lists, we must cross-reference the different PRs for a single project and then leave the one with the "new font" info as the more relevant to track it as a whole.
Sounds like something manage-traffic-jam should do automatically? Atm, I think it only keeps the latest pr. Ideally, a filepath should list multiple prs if they exist e.g for Kalnia we'd have:
Multiple Categories:
ofl/kalnia # [https://github.com/google/fonts/pull/6879, https://github.com/google/fonts/pull/7008]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m4rc1e tracking several PRs for the same directory path is not efficient and is creating confusion. It also mean moving more PRs through the pipeline than we need, and so wasting time when checking the servers. It also make it harder to check if the list is correctly reflecting the board. It doesn’t take long to remove the PRs we don’t want to track from the board so any team members can understand the list. Actually the author of the PR can do it directly when making the PR.
« Multiple categories » could be a solution but it wouldn’t reflect if it’s a new font or an upgrade. I actually don’t think the tool could ever choose between multiple PRs and know which one would have the most relevant changes when looking case by case.. So all in all I don’t think it can be automated by the tool, and other stuff are more priority for the tool than this really simple task of removing a PR from the board. But let’s say, even if it could be automated, it can be talked over at the next meeting and is no reason for blocking the completion of the list making. It would have taken less time to remove the additional PR for Kalnia than whatever we going through right through multiple communication channels.
So please, @eliheuer can you finish the task and we’ll discuss the rest later?
- you still need to ask Rod in the chat about all these PRs for noto emoji: do we need to track them at all?
- There are multiple PRs concerning this repo only, therefore they can be removed from the board
- Removing the unnecessary PRs so only one per font directory per server is properly tracked (to allow clear categorization in the list)
- Inform in the Khojki PRs to Simon that the lang update didn't solve the tofu problem.
- Label this PR "Tool/workflow/repo" and remove it from the board
- Assign Chris to review and merge when everything looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don’t think the tool could ever choose between multiple PRs and know which one would have the most relevant changes when looking case by case.. So all in all I don’t think it can be automated by the tool,
SGTM. I'll leave as is.
catalog/designers/jenskutilek # https://github.com/google/fonts/pull/6935 | ||
|
||
# Metadata / Description / License | ||
ofl/sixtyfour # https://github.com/google/fonts/pull/7033 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inclusion of Homecomputer fonts (Sixtyfour and Workbench) should be tracked by PRs # 7025 and 7026, which are the ones introducing them as New Fonts
.
#7033 was only introducing the minisite Url, and that change would be included by the PRs moving the ofl dirs of the fonts. This PR can be removed from the Traffic Jam board then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above for these fonts, hence the previous comment.
I made a new PR that depricates this and fixed all the issues discussed here in that PR: #7063 |
No description provided.