search

googlefonts / fontbakery-dashboard

A library-scale web dashboard for Font Bakery, no longer developed
Apache License 2.0
28 stars 10 forks source link

Feedback Review Dispatcher No. 2 2019 Oct-7 #131

Open graphicore opened 4 years ago

graphicore commented 4 years ago

One problem that occured is recreated and filed at #130

graphicore commented 4 years ago

@vv-monsalve ran this process which had some issues: https://fontbakery.graphicore.de/dispatcher/process/9542ad3c-7367-42d0-8eed-51468f9e3429

graphicore commented 4 years ago

We had some issues with gRPC Error: 14 UNAVAILABLE: TCP Read failed seems like this is related: grpc/grpc-node#692 and this it's about reconnecting to a service i.e. if it's pod was restarted.

graphicore commented 4 years ago

One suggestion was the possibility to only update one file e.g. DESCRIPTION.en_us.html. Because that's likely in the interest of a Designer, who is happy with the font version on GF but not with the info displayed with the font on Google Fonts.

Also, regarding DESCRIPTION.en_us.html we had a case where the file was called Description.en_us.html, but case sensitivity is an issue with this file. My suggestion was that the tool could try to detect cases where there are case ambiguities and warn or inform the user about it. We could also be less strict, but I don't think that's the best way, being specific makes sense to reduce code complexity (though, creating that detection and warning code is also more complex than not doing so).

graphicore commented 4 years ago

We could have a Font Bakery profile for upstream repos, maybe together with the configuration that we make with the spreadsheet in the dispatcher. :man_shrugging:

vv-monsalve commented 4 years ago

A couple more things were suggested:

vv-monsalve commented 4 years ago

The description file name is already adjusted. I repeated the process and the file was correctly added to the Files Package. DiffBrowsers also worked and the downloaded folder name (Browser Diffs_FamilyName) is working good. Although PR returned an Error because of a PR already exist

GitHub PR Server can't dispatch Pull Request. ERROR Error: (422) Validation Failed [POST url: https://api.github.com/repos/graphicore/googleFonts/pulls] Errors: [custom] PullRequest: A pull request already exists for graphicore:Font_Bakery_Dispatcher_2019_10_07_ofl_bebasneue. designated GitHub branch page

What should be done in cases like this? How can the user delete or modify the previous PR? E.g. if the user goes through the entire process but realizes about something that needs to be changed or modified?

vv-monsalve commented 4 years ago

About the possibility to only update one file, after inspecting the FontBakery documentation about DESCRIPTION file I think this could be particularly necessary given this:

Is this a proper HTML snippet?

When packaging families for google/fonts, if there is no DESCRIPTION.en_us.html file, the add_font.py metageneration tool will insert a dummy description file which contains invalid html. This file needs to either be replaced with an existing description file or edited by hand.

graphicore commented 4 years ago

Hmm, that's what happens with a new family, and there we'd update the upstream repository to include a DESCRIPTION.en_us.html but then still need the complete files package with all fonts to create that new entry.

For an update however, where the font files don't need to change on GF, but the upstream font files have diverged from the GF font files, this would be the use case. If the font files in the upstream have not changed, it doesn't matter if we include them or not.

graphicore commented 4 years ago

DiffBrowsers also worked and the downloaded folder name (Browser Diffs_FamilyName) is working good.

I can't reproduce the issue right now either, but I did in #130

Although PR returned an Error because of a PR already exist [...] What should be done in cases like this? How can the user delete or modify the previous PR? E.g. if the user goes through the entire process but realizes about something that needs to be changed or modified?

The user can close the existing PR and run the Task again. However, since in the case of another dispatch attempt on the same day, the branch is changed (force pushed), and also the existing PR is updated, despite of the error message "A pull request already exists". I guess we could either close the PR and create another one OR just add a comment with the new dispatcher report to the existing PR, which I like more, because it keeps the discussion in one place. In that latter case, date-stamps for branch names should be avoided as well i.e. Font_Bakery_Dispatcher_ofl_bebasneue instead of Font_Bakery_Dispatcher_2019_10_07_ofl_bebasneue.

graphicore commented 4 years ago

Another Idea we had: it would be nice to have a control flow diagram of the current dispatcher process. It would also be nice to be able to generate it automatically. Since some of the "edges" i.e. the path between two "nodes" are decided in javascript code it's not simple. However, it could be made by tooling this feature specifically into the Process framework. Just need to think about it.