flattenthecurve / guide

https://www.flattenthecurve.com
Creative Commons Attribution 4.0 International
38 stars 33 forks source link

Refactor import_languages.rb script and allow for optional pending-sci-review #357

Closed rousik closed 4 years ago

rousik commented 4 years ago

The core of this change is to allow inserting pending-sci-review notice to act_and_prepare articles. To allow for this, the structure of the language import script has been refactored so that this could be done relatively cleanly.

Here's the brief summary of the changes:

  1. added --sci-review-notice flag which will trigger insertion of pending-sci-review notices for translations that are not marked as reviewed in Lokalise
  2. Refactoring of the code that pulls translations from Lokalise. The code has been moved to a function which allows us to specify whether we want to retrieve translations or only reviewed keys. The code returns JSON instead of generating local files under _translations/ and the method is called twice by default to find out which keys are not scientifically reviewed.
  3. Files under _translations/ are now generated from in-memory json. This generates almost the same content but results in some whitespace changes.
  4. If SINGLE_LANG is specified, request to Lokalise will only fetch the translations for given language (minor optimization).

As far as the sci-review-pending notice goes, this will get inserted into the translated text after the last header line (or at the beginning of the file if no headers are found). This is somewhat strange but it seems to be the only way to get the notice in the right place. Unless we make some dramatic changes to the styling of these markdown files that would remove all headers this should work just fine.

github-actions[bot] commented 4 years ago

This pull request is being automatically deployed with now-deployment

Built with commit 52ec45137b3454177cef0e67fd34bea439040bec

✅ Preview: https://guide-preview-2arpyjno5.now.sh

bcardiff commented 4 years ago

I'm not 100% sure but it seems that this broke something in the CI. All the "Preview translations" workflows are failing now. https://github.com/flattenthecurve/guide/actions?query=workflow%3A%22Preview+translations%22

Maybe is the user context https://github.com/flattenthecurve/guide/commit/76e2a7b4656e43203dea0a94e2d996c0842cf592#diff-0fff584a8af28d53dbf23320ade81ee0R12 ?

Failing since https://github.com/flattenthecurve/guide/runs/622727974?check_suite_focus=true Last success https://github.com/flattenthecurve/guide/runs/621149872?check_suite_focus=true

rousik commented 4 years ago

Yep. I'm aware of that. I have mentioned this on Slack earlier today. It seems to me that ruby caching has gone bad. I don't understand how that works so I don't know if I can debug but I discovered that not using the docker-compose.ci.yml overrides does fix the problem (but also breaks the caching).

Do you have insights into why the caching logic may be failing? I did change the imports for the script recently.

On Wed, Apr 29, 2020, 14:39 Brian J. Cardiff notifications@github.com wrote:

I'm not 100% sure but it seems that this broke something in the CI. All the "Preview translations" workflows are failing now.

https://github.com/flattenthecurve/guide/actions?query=workflow%3A%22Preview+translations%22

Maybe is the user context 76e2a7b#diff-0fff584a8af28d53dbf23320ade81ee0R12 https://github.com/flattenthecurve/guide/commit/76e2a7b4656e43203dea0a94e2d996c0842cf592#diff-0fff584a8af28d53dbf23320ade81ee0R12 ?

Failing since https://github.com/flattenthecurve/guide/runs/622727974?check_suite_focus=true Last success https://github.com/flattenthecurve/guide/runs/621149872?check_suite_focus=true

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/flattenthecurve/guide/pull/357#issuecomment-621450903, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYSCGA6ONZY22REUCFWMUTRPCGAVANCNFSM4MMZMYOA .

bcardiff commented 4 years ago

Since removing docker-compose.ci.yml solves the issue and the user on the docker run changed recently I would bet that the stored cache has wrong pemissions.

I would try changing both occurences in https://github.com/flattenthecurve/guide/blob/master/.github/workflows/preview-translations.yml#L14-L16 of ${{ runner.os }}-gems- to ${{ runner.os }}-gems-v2- to havoc the cache and see how it goes.

rousik commented 4 years ago

K, will do tomorrow.

On Wed, Apr 29, 2020, 18:21 Brian J. Cardiff notifications@github.com wrote:

Since removing docker-compose.ci.yml solves the issue and the user on the docker run changed recently I would bet that the stored cache has wrong pemissions.

I would try changing both occurences in https://github.com/flattenthecurve/guide/blob/master/.github/workflows/preview-translations.yml#L14-L16 of ${{ runner.os }}-gems- to ${{ runner.os }}-gems-v2- to havoc the cache and see how it goes.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/flattenthecurve/guide/pull/357#issuecomment-621538557, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYSCGHSPM5FFFHRUQ2SO4TRPDAAFANCNFSM4MMZMYOA .

rousik commented 4 years ago

It seems that the change of uid was not the right thing to do so I'm reverting that in https://github.com/flattenthecurve/guide/pull/383 (awaiting review)

On Wed, Apr 29, 2020 at 6:35 PM Jan Rouš rousik@gmail.com wrote:

K, will do tomorrow.

On Wed, Apr 29, 2020, 18:21 Brian J. Cardiff notifications@github.com wrote:

Since removing docker-compose.ci.yml solves the issue and the user on the docker run changed recently I would bet that the stored cache has wrong pemissions.

I would try changing both occurences in https://github.com/flattenthecurve/guide/blob/master/.github/workflows/preview-translations.yml#L14-L16 of ${{ runner.os }}-gems- to ${{ runner.os }}-gems-v2- to havoc the cache and see how it goes.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/flattenthecurve/guide/pull/357#issuecomment-621538557, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYSCGHSPM5FFFHRUQ2SO4TRPDAAFANCNFSM4MMZMYOA .

rousik commented 4 years ago

Hey Brian, what is the best way to get in touch with you if I want to ask some questions? I assume you're not on the flattenthecurve slack workspace. Feel free to shoot me email at rousik gmail if you're willing.

On Thu, Apr 30, 2020 at 2:40 PM Jan Rouš rousik@gmail.com wrote:

It seems that the change of uid was not the right thing to do so I'm reverting that in https://github.com/flattenthecurve/guide/pull/383 (awaiting review)

On Wed, Apr 29, 2020 at 6:35 PM Jan Rouš rousik@gmail.com wrote:

K, will do tomorrow.

On Wed, Apr 29, 2020, 18:21 Brian J. Cardiff notifications@github.com wrote:

Since removing docker-compose.ci.yml solves the issue and the user on the docker run changed recently I would bet that the stored cache has wrong pemissions.

I would try changing both occurences in https://github.com/flattenthecurve/guide/blob/master/.github/workflows/preview-translations.yml#L14-L16 of ${{ runner.os }}-gems- to ${{ runner.os }}-gems-v2- to havoc the cache and see how it goes.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/flattenthecurve/guide/pull/357#issuecomment-621538557, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYSCGHSPM5FFFHRUQ2SO4TRPDAAFANCNFSM4MMZMYOA .

bcardiff commented 4 years ago

HI @rousik a mention in github should be enough.

My current allocations are not in this project. nditada and matiasgarciaisaia are coworkers and can reach me in our slack if needed. I noticed the errors since I set up the build started receiving some alerts in my inbox :-).