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

Feature/aris/issue_3902_add_scripts_to_ci #4216

Merged
merged 4 commits into from
Oct 12, 2021

Conversation

ariskotsomitopoulos
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos commented Oct 11, 2021

Fixes #3902

Implement a new github workflow to run :

./tools/import_emojis.py
./tools/import_sas_strings.py

every Monday at 00:00 UTC and create two PRs:

  1. Sync Emojis
  2. Sync SAS Strings

How the automated cron action behaves:

  • If there are changes (i.e. a diff exists with the checked-out base branch), the changes will be pushed to a new branch and a pull request created.

  • If there are no changes (i.e. no diff exists with the checked-out base branch), no pull request will be created and the action exits silently.

  • If a pull request already exists and there are no further changes (i.e. no diff with the current pull request branch) then the action exits silently.

  • If a pull request exists and new changes on the base branch make the pull request unnecessary (i.e. there is no longer a diff between the pull request branch and the base), the pull request is automatically closed. Additionally, if delete-branch is set to true the branch will be deleted.

Further workflows(Like Unit tests, Builds APK etc) on the generated PRs are disabled, we can enable them with the appropriate configuration if needed

@github-actions
Copy link

github-actions bot commented Oct 11, 2021

Unit Test Results

  44 files  +2    44 suites  +2   57s ⏱️ +15s
  87 tests +4    87 ✔️ +4  0 💤 ±0  0 ±0 
228 runs  +8  228 ✔️ +8  0 💤 ±0  0 ±0 

Results for commit 2b2f5be. ± Comparison against base commit 117fa71.

♻️ This comment has been updated with latest results.

@ariskotsomitopoulos ariskotsomitopoulos changed the title IGNORE THIS PR IS FOR TESTING Feature/aris/issue_3902_add_scripts_to_ci Oct 11, 2021
…ings. It will run every Monday at 00:00. It will open two PRs and will be able to optimal update/delete them according to changes with the base branch
@ariskotsomitopoulos ariskotsomitopoulos force-pushed the feature/aris/issue_3902_add_scripts_to_ci branch from d92d5fa to 6cee887 Compare October 11, 2021 14:52
@ariskotsomitopoulos ariskotsomitopoulos marked this pull request as ready for review October 11, 2021 14:52
changelog.d/4216.misc Outdated Show resolved Hide resolved
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

Can I ask you to remove the two lines referencing the 2 scripts in this file?

@bmarty
Copy link
Member

bmarty commented Oct 11, 2021

Further workflows(Like Unit tests, Builds APK etc) on the generated PRs are disabled, we can enable them with the appropriate configuration if needed

Where is it disabled?

@ariskotsomitopoulos
Copy link
Contributor Author

Further workflows(Like Unit tests, Builds APK etc) on the generated PRs are disabled, we can enable them with the appropriate configuration if needed

Where is it disabled?

It is disabled by default, it needs some modifications to support it, I can enhance the workflow to support it, do you think we need the tests on those PRs?

@ouchadam
Copy link
Contributor

if these workflows create a PR, won't those PRs run the default set of tasks? https://github.com/vector-im/element-android/blob/develop/.github/workflows/tests.yml#L4 there's no filtering (from what I can tell!)

@ariskotsomitopoulos
Copy link
Contributor Author

https://github.com/vector-im/element-android/blob/develop/.github/workflows/tests.yml#L4

I think they will not, they need special access for that (github access token), I asked @bmarty in the comment above if we want the tests to run in order to enhance the workflow!

@bmarty
Copy link
Member

bmarty commented Oct 12, 2021

I agree with @ouchadam , every PRs should be checked by the regular CI. Looking at #4217, this is not the case, only Buildkite has been run... But this is probably fine for what the 2 tools are doing. Anyway we will still review the change before merging such PR

@@ -0,0 +1,69 @@
name: Sync Data From External Sources
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arg, there is a typo in the file name :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

soruces

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch !

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename the file to fix the typo and we can merge the PR.

Thanks!

@bmarty bmarty merged commit 0292afb into develop Oct 12, 2021
@bmarty bmarty deleted the feature/aris/issue_3902_add_scripts_to_ci branch October 12, 2021 12:53
@ariskotsomitopoulos
Copy link
Contributor Author

I agree with @ouchadam , every PRs should be checked by the regular CI. Looking at #4217, this is not the case, only Buildkite has been run... But this is probably fine for what the 2 tools are doing. Anyway we will still review the change before merging such PR

I agree with both approaches! If we want all tests to run I can enhance it anytime, no worries

@bmarty
Copy link
Member

bmarty commented Oct 12, 2021

Thanks for the PR.

Can I ask you to remove the two lines referencing the 2 scripts in this file?

It has been done directly on the develop branch IFAIU in d263409. It could have been part of this PR.

@ariskotsomitopoulos
Copy link
Contributor Author

Thanks for the PR.
Can I ask you to remove the two lines referencing the 2 scripts in this file?

It has been done directly on the develop branch IFAIU in d263409. It could have been part of this PR.

You are totally right! It might be good to add a Branch Protection Rule to prevent this to happen again.

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.

Let GitHub works for us and create PR for changes which comes after running internal scripts
3 participants