Skip to content

Reenable main tests on Crowdin PRs#6076

Merged
tramuntanal merged 2 commits intodevelopfrom
chore/run-main-tests-on-crowdin
May 8, 2020
Merged

Reenable main tests on Crowdin PRs#6076
tramuntanal merged 2 commits intodevelopfrom
chore/run-main-tests-on-crowdin

Conversation

@mrcasals
Copy link
Copy Markdown
Contributor

@mrcasals mrcasals commented May 7, 2020

🎩 What? Why?

This PR re-enables the main CI builds (and only those) for Crowdin PRs, so that we can detect problems with the JS bundles.

Currently develop sdeems to be failing because of this.

📌 Related Issues

📋 Subtasks

None

@mrcasals mrcasals self-assigned this May 7, 2020
@mrcasals mrcasals changed the title Chore/run main tests on crowdin Reenable main tests on Crowdin PRs May 7, 2020
@mrcasals
Copy link
Copy Markdown
Contributor Author

mrcasals commented May 7, 2020

@decidim/core can you review this PR?

@tramuntanal
Copy link
Copy Markdown
Contributor

tramuntanal commented May 7, 2020

Hi @mrcasals thanks for the PR!

Just now I've been working on this. I was thinking to protocolize always building js bundles before merging Crowdin PRs.
Crowdin is now configured to synchronize every 1 hour, and the CI test queue is in part slow due to l10n tests. When there's intense translation activity like last week, the wainting time for a single test was longer than 24h.

So what do we prefer to have tests for Crowdin PRs or @decidim/core manually build the JS bundles before merging them?
@andreslucena what are your thoughts on this?

@tramuntanal tramuntanal mentioned this pull request May 7, 2020
Closed
6 tasks
@mrcasals
Copy link
Copy Markdown
Contributor Author

mrcasals commented May 7, 2020

But this already happened before 🤔

Bundles only need to be remade in these cases:

  1. We modify the JS. This happens when devs send PRs, and they fix it themselves.
  2. We modify the comments locales. This happens both on Crowdin PRs and devs. Devs fix it temselves, but Crowdin PRs need to be fixed before merging.

I think simply adding the tests for the Crowdin PRs will detect those changes while keeping the CI builds queue clean.

The alternative is to set Crowdin to 24h no matter what, reenable CI builds for those PRs and force sync before releasing new versions.

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented May 8, 2020

@andreslucena what are your thoughts on this?

I don't really have any thoughts regarding this.

The alternative is to set Crowdin to 24h no matter what

I'm ok with that. The only case where I think we may need translations before 24h is when we're making a release and on those cases we could sync manually.

@tramuntanal tramuntanal merged commit 4fb96a4 into develop May 8, 2020
@tramuntanal tramuntanal deleted the chore/run-main-tests-on-crowdin branch May 8, 2020 06:16
@tramuntanal
Copy link
Copy Markdown
Contributor

tramuntanal commented May 8, 2020

I've configured Crowdin to synchronize every 24h.

@tramuntanal tramuntanal mentioned this pull request May 8, 2020
Closed
ace pushed a commit to aspgems/decidim that referenced this pull request May 12, 2020
Merge branch 'feature/initiatives_search_fo_new_design' into deploy
a637940 
* feature/initiatives_search_fo_new_design:
  Updates changelog
  Harmonizes the design of initiatives search in FO
  New question type "Matrix" in questionnaires (decidim#5948)
  Add filter options to Timeline and Activity tabs (decidim#5845)
  Remove relations between user and spaces on destroy account command (decidim#6041)
  Explain how to initialize a custom oauth2 client provider (decidim#6055)
  Reenable main tests on Crowdin PRs (decidim#6076)
  Enum and readonly component settings (decidim#6001)
  New Crowdin translations (decidim#6066)
  Add missing notifications (decidim#5906)
@tramuntanal tramuntanal mentioned this pull request Jul 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tramuntanal tramuntanal tramuntanal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mrcasals mrcasals

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mrcasals @tramuntanal @andreslucena