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

refactor: Rewrite scripts to Kotlin #1246

Merged
merged 99 commits into from
Nov 2, 2020

Conversation

piotradamczyk5
Copy link
Contributor

@piotradamczyk5 piotradamczyk5 commented Oct 16, 2020

Fixes #1178

Test Plan

How do we know the code works?

Scripts are executed properly on macOS, Linux, Windows.
To run scripts use proper .sh or .bat file
Kotlin v1.4 must be installed

Warning

Kotlin scripts have problems with caching, often scripts don't notice changes so a good way to have always "fresh" script is reset scripts cache on every run. This problem and solution is documented here.

Checklist

  • ./test_runner/bash/update_flank.sh
  • ./test_runner/src/test/kotlin/ftl/fixtures/tmp/gohello/build.sh (this is same as ./test_projects/gohello/build.sh)
  • ./flank-scripts/bash/buildFlankScripts.sh
  • ./test_runner/src/main/resources/binaries/update.sh
  • ./test_projects/ops.sh (many functions)
  • ./test_projects/gohello/build.sh (with possibility to change gohello path)
  • ./test_projects/android/ops.sh (many functions)
  • ./test_projects/android/bash/test_filters.sh.
  • ./test_projects/ios/EarlGreyExample/ops.sh (many functions)
  • ./test_projects/ios/EarlGreyExample/build_example.sh
  • ./test_projects/ios/EarlGreyExample/universal_framework.sh
  • ./test_projects/ios/EarlGreyExample/run_ftl_local.sh
  • ./test_projects/ios/EarlGreyExample/build_ftl.sh
  • ./firebase_apis/generate_java_client.sh
  • ./firebase_apis/update_api_json.sh
  • Make PR on Flank/binaries with link to scripts

@piotradamczyk5 piotradamczyk5 changed the title #1178 rewrite scripts to kotlin refactor: Rewrite scripts to Kotlin Oct 16, 2020
@codecov-io
Copy link

codecov-io commented Oct 16, 2020

Codecov Report

Merging #1246 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1246      +/-   ##
============================================
- Coverage     79.66%   79.65%   -0.01%     
  Complexity      724      724              
============================================
  Files           235      235              
  Lines          4489     4498       +9     
  Branches        771      773       +2     
============================================
+ Hits           3576     3583       +7     
- Misses          514      515       +1     
- Partials        399      400       +1     

@adamfilipow92
Copy link
Contributor

.bat scripts tested on windows and works

@piotradamczyk5 piotradamczyk5 force-pushed the #1178-rewrite_scripts_to_kotlin branch from 3ba1862 to 3da60a3 Compare October 19, 2020 14:20
@piotradamczyk5 piotradamczyk5 marked this pull request as ready for review October 19, 2020 14:21
@piotradamczyk5
Copy link
Contributor Author

PR for binaries

flank-bash/scripts/android.ops.main.kts Outdated Show resolved Hide resolved
flank-bash/scripts/updateLibs/updateLlvm.main.kts Outdated Show resolved Hide resolved
flank-bash/scripts/updateLibs/updateSwift.main.kts Outdated Show resolved Hide resolved
@bootstraponline bootstraponline force-pushed the #1178-rewrite_scripts_to_kotlin branch 2 times, most recently from a32bccd to 0e64c91 Compare October 20, 2020 11:11
@piotradamczyk5 piotradamczyk5 marked this pull request as draft October 20, 2020 11:44
@piotradamczyk5
Copy link
Contributor Author

@Mergifyio rebase

@piotradamczyk5 piotradamczyk5 marked this pull request as ready for review October 22, 2020 13:58
@mergify
Copy link

mergify bot commented Oct 22, 2020

Command rebase: success

Branch has been successfully rebased

@bootstraponline bootstraponline force-pushed the #1178-rewrite_scripts_to_kotlin branch from ba927aa to f45a3c5 Compare October 22, 2020 13:59
@piotradamczyk5 piotradamczyk5 requested a review from Sloox October 22, 2020 14:01
@bootstraponline bootstraponline force-pushed the #1178-rewrite_scripts_to_kotlin branch 2 times, most recently from b55718b to e95bb0d Compare October 22, 2020 15:27
@pawelpasterz
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link

mergify bot commented Oct 23, 2020

Command refresh: success

Pull request refreshed

@bootstraponline bootstraponline force-pushed the #1178-rewrite_scripts_to_kotlin branch 2 times, most recently from c350f97 to 511235b Compare October 26, 2020 07:32
@bootstraponline bootstraponline force-pushed the #1178-rewrite_scripts_to_kotlin branch from b897aab to c065c0b Compare October 26, 2020 11:57
@pawelpasterz
Copy link
Contributor

This problem and solution is documented here.

I think link is not valid anymore

Copy link
Contributor

@jan-goral jan-goral left a comment

Choose a reason for hiding this comment

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

Implementing logic code directly inside CliKt commands breaks the separation of concerns, and makes it harder to read, because of object-oriented API overhead. But this is a huge PR, also required for windows os. I can consider to approve it now and refactor a little later.

@adamfilipow92 adamfilipow92 marked this pull request as draft October 27, 2020 16:01
@piotradamczyk5 piotradamczyk5 force-pushed the #1178-rewrite_scripts_to_kotlin branch from 19cb73e to c6cd1f2 Compare October 29, 2020 14:08
@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2020

Timestamp: 2020-11-02 12:24:10
Buildscan url for ubuntu-workflow run 341645329
https://gradle.com/s/ve2ebhtklhesw

@adamfilipow92 adamfilipow92 marked this pull request as ready for review October 30, 2020 12:06
@piotradamczyk5
Copy link
Contributor Author

I will create a ticket to fix the integration test on Windows later on

@piotradamczyk5 piotradamczyk5 merged commit eba5ffe into master Nov 2, 2020
@piotradamczyk5 piotradamczyk5 deleted the #1178-rewrite_scripts_to_kotlin branch November 2, 2020 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port bash scripts to Kotlin
7 participants