Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

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

Open
graphicore opened this issue Oct 7, 2019 · 10 comments
Open

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

graphicore opened this issue Oct 7, 2019 · 10 comments

Comments

@graphicore
Copy link
Contributor

graphicore commented Oct 7, 2019

One problem that occured is recreated and filed at #130

@graphicore
Copy link
Contributor Author

graphicore commented Oct 7, 2019

@graphicore
Copy link
Contributor Author

graphicore commented Oct 7, 2019

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
Copy link
Contributor Author

graphicore commented Oct 7, 2019

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
Copy link
Contributor Author

We could have a Font Bakery profile for upstream repos, maybe together with the configuration that we make with the spreadsheet in the dispatcher. 🤷‍♂️

@vv-monsalve
Copy link

vv-monsalve commented Oct 7, 2019

A couple more things were suggested:

  • To add some status message or sign while the DiffBrowsers is working, so the user is informed about it.
  • To check with Marc which file must be included in Files Package.

@vv-monsalve
Copy link

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

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants