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

[GitHub CI] Revert back to Ruby Danger as Danger JS is not working properly #1650

Merged
merged 7 commits into from
Aug 31, 2019
Merged

Conversation

Kaspik
Copy link
Contributor

@Kaspik Kaspik commented Aug 28, 2019

  • update danger.yml to run as a script
  • add ruby danger step to build.sh file and run Danger on every PR
  • remove Danger JS as it's not working as it should

Protected branches - Danger JS step has to be replaced as require with the Run Danger step that is now working.

Important:

Original Danger Ruby (implemented here) takes about 40sec where the new Danger JS that is right now on master takes about 4 mins.

Add name to the script run phase
… for push for tests and for PR for tests + danger, run ruby danger as part of the PR check
This reverts commit 839ba3a.
Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

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

Great point on using the old Ruby file by adding a new mode to build.sh! Thank you!

I have one question: Should we just update the existing danger.yml file to run ./build.sh danger and keep ci.yml as is? That means we won't need to copy all other jobs over like what you propose in the new ci_pr.yml file?

@nguyenhuy
Copy link
Member

nguyenhuy commented Aug 29, 2019

For reference, #1635 introduced the Danger JS file and this PR reverts it.

@Kaspik
Copy link
Contributor Author

Kaspik commented Aug 29, 2019

Actually, that's a good idea. Updated PR!

buildsh:
strategy:
matrix:
mode: [danger]
Copy link
Member

Choose a reason for hiding this comment

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

With only one configuration, using a matrix is a bit overkill since you can just configure the job directly (like this). Please feel free to follow up on another PR but not a big deal otherwise.

@nguyenhuy nguyenhuy merged commit d0ab404 into TextureGroup:master Aug 31, 2019
@nguyenhuy
Copy link
Member

Btw, thanks again for fixing Danger!

@Kaspik
Copy link
Contributor Author

Kaspik commented Aug 31, 2019

Thank you for merging it!
Yeah, I copied the main file. Will take a look!

@Kaspik Kaspik deleted the patch-1 branch August 31, 2019 15:24
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.

None yet

2 participants