-
Notifications
You must be signed in to change notification settings - Fork 32
chore: removed travis yml and added git action support #469
8000 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
Conversation
Pull Request Test Coverage Report for Build 1979
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple of suggestions.
.github/workflows/java.yml
Outdated
echo "$GITHUB_CONTEXT" | ||
home/runner/travisci-tools/trigger-script-with-status-update.sh | ||
test: | ||
runs-on: macos-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why macos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is not fixed yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a nit
.github/workflows/java.yml
Outdated
BUILD_NUMBER: ${{ github.run_id }} | ||
GITHUB_CONTEXT: ${{ toJson(github) }} | ||
EVENT_NAME: ${{ github.event_name }} | ||
REPOSITORY: ${{ github.repository }} | ||
SHA: ${{ github.sha }} | ||
TRAVIS_PULL_REQUEST_SHA: ${{ github.event.pull_request.head.sha }} | ||
TRAVIS_PULL_REQUEST: ${{ github.event.pull_request.number }} | ||
EVENT_MESSAGE: ${{ github.event.message }} | ||
HEAD_REF: ${{ github.head_ref }} | ||
REF_NAME: ${{ github.ref_name }} | ||
secrets: | ||
CI_USER_TOKEN: ${{ secrets.CI_USER_TOKEN }} | ||
TRAVIS_COM_TOKEN: ${{ secrets.TRAVIS_COM_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see factoring out to "integration_test.yml" looks good, but not much gain (more overhead than sharing). I'll leave it up to you. I guess you already tried, but wondering if we cannot use ${{ github }} directly in "integration_test.yml" instead of passing as input parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i actually asked for it that cleans-up our main.yaml file.
Regarding your suggestion we tried but github is very specific to passing secrets info to actions, that's why this was not done.
.github/workflows/java.yml
Outdated
echo "$GITHUB_CONTEXT" | ||
home/runner/travisci-tools/trigger-script-with-status-update.sh | ||
test: | ||
runs-on: macos-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is not fixed yet.
@mnoman09 I tried to release android-sdk with actions flow first time, and found a couple of errors. We can do the same fix for java-sdk.
|
@mnoman09 Also found that "publish" task is launched even before unit tests completed. Either we can make it wait until all tests completed or we can skip them for publish (since we already done the same tests before publish). |
@@ -0,0 +1,16 @@ | |||
name: Source clear | |||
|
|||
on: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add master branch here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Summary
NOTE:
Passed build link:
Test plan
All tests should pass