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

Update to lists for 1st of December #7053

Closed
wants to merge 1 commit into from

Conversation

eliheuer
Copy link
Collaborator

@eliheuer eliheuer commented Dec 1, 2023

No description provided.

@@ -1,13 +1,585 @@
# New
ofl/afacad # https://github.com/google/fonts/pull/6881
ofl/kalnia # https://github.com/google/fonts/pull/6879
Copy link
Collaborator

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

Copy link
Contributor

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.

Copy link
Collaborator Author

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?
Screenshot 2023-12-04 at 8 59 44 AM

Copy link
Collaborator Author

@eliheuer eliheuer Dec 4, 2023

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.

Copy link
Contributor

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.

Copy link
Collaborator

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.

Copy link
Collaborator

@m4rc1e m4rc1e Dec 4, 2023

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]

Copy link
Contributor

@RosaWagner RosaWagner Dec 5, 2023

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

Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator

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.

@eliheuer
Copy link
Collaborator Author

eliheuer commented Dec 5, 2023

I made a new PR that depricates this and fixed all the issues discussed here in that PR: #7063

@eliheuer eliheuer closed this Dec 5, 2023
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.

4 participants