-
Notifications
You must be signed in to change notification settings - Fork 30
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
chore: removed travis yml and added git action support #454
Conversation
874565f
to
2589339
Compare
echo "SDK_BRANCH=${{ github.ref_name }}" >> $GITHUB_ENV | ||
echo "TRAVIS_BRANCH=${{ github.ref_name }}" >> $GITHUB_ENV |
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.
these variables doesn't seem correct to set with refname.
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.
ref_name
has the The branch or tag name that triggered the workflow run.
as per the official documentation, in case build was not triggered by pull request. This is required by travisci-tools/trigger-script-with-status-update.sh
.
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.
remove it, we don't have any relation with the current repo to trigger-script-with-status-update.sh
RELEASE: ${{ secrets.RELEASE }} | ||
if: "${{ env.PREP == '' && env.RELEASE == '' }}" | ||
run: | | ||
echo "$GITHUB_CONTEXT" |
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.
no need of it.
ee964ca
to
39b7edd
Compare
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 all the previous issue cleaned up. I have a few more suggestions.
Scripts/run_prep.sh
Outdated
#!/bin/bash -e | ||
set -e |
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.
Do we need both "bash -e" and "set -e"?
.github/workflows/swift.yml
Outdated
xcode-version: '12.4' | ||
- name: Push to cocoapods.org | ||
env: | ||
VERSION: 3.10.1 |
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.
This SDK version may be confusing - one for "prepare-for-release" is defined in the workflow-call and the same for "release" is defined in this top level.
It'll be great if we can find a way to share a single variable at the top level and pass it to "prepare-for-release" as a parameter.
.github/workflows/swift.yml
Outdated
env: | ||
VERSION: 3.10.1 | ||
GITHUB_TOKEN: ${{ secrets.CI_USER_TOKEN }} | ||
TRAVIS_BRANCH: ${{ github.ref_name }} |
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.
Do we still use "TRAVIS" in the names? :)
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.
Removed all env variables with previs TRAVIS_
leaving only those which are required by trigger-script-with-status-update
script of travis-ci-tools
.
- id: prepare_for_release | ||
name: Prepare for release | ||
env: | ||
VERSION: 3.10.1 |
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.
This value (SDK release version) should be passed as a parameter from the top level
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 looked into it and have tried to make it a global env. For using reusable_workflows
, we cannot pass env variables to them. Because of this, version is declared twice in swift.yml
.
# Link and create simulators from older xcode versions which are not part of the current xcode version | ||
sudo ln -s /Applications/Xcode_$SIMULATOR_XCODE_VERSION.app/Contents/Developer/Platforms/$os_folder/CoreSimulator/Profiles/Runtimes/$OS_TYPE.simruntime /Library/Developer/CoreSimulator/Profiles/Runtimes/$OS_TYPE\ $OS.simruntime | ||
xcrun simctl create "custom-device" "com.apple.CoreSimulator.SimDeviceType.$name" "com.apple.CoreSimulator.SimRuntime.$OS_TYPE-$os" | ||
CUSTOM_SIMULATOR="$(instruments -s devices | grep -m 1 'custom-device' | awk -F'[][]' '{print $2}')" | ||
else | ||
echo ".devices.\"com.apple.CoreSimulator.SimRuntime.${PLATFORM/ Simulator/}-${OS/./-}\"" > /tmp/jq_file | ||
CUSTOM_SIMULATOR=$( xcrun simctl list --json devices | jq -f /tmp/jq_file | jq -r '.[] | select(.name==env.NAME) | .udid' ) | ||
fi |
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.
It looks like you spent a lot of efforts to figure out all these mappings :) Not a good support from github. I'm concerned if we need to adjust the versions and paths whenever Apple releases new iOS versions.
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.
Since we are using runs-on: macos-10.15
, these values will not change. Simulators and xcode versions are released along with macos
versions by git actions. If we intend to move to newer versions of simulators, we would just need to update the runs-on:
value and update the simulator versions as per the new documentation in unit_tests.yml
.
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 great! One last change we can consider.
Hey @jaeopt , i am unable to see the change request 👍 |
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 don't know what happened to my prev comments :) Here we go again.
.github/workflows/swift.yml
Outdated
@@ -53,7 +53,7 @@ jobs: | |||
if: "${{ github.event.inputs.PREP == 'true' && github.event_name == 'workflow_dispatch' }}" | |||
uses: optimizely/swift-sdk/.github/workflows/prepare_for_release.yml@yasir/gitAction | |||
with: | |||
VERSION: ${{ env.VERSION }} | |||
VERSION: 3.10.1 # env variables cannot be used for reusable workflows |
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 now see the limitation of sharing var. I wonder if we can bring "prepare_for_release" back to "swift.yml" and share a single VERSION for both "release" and "prepare_for_release". Any reason we keep only one of them in the separate workflow?
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
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
release
andpre-release
sections.Test plan
All tests should pass
Result
https://github.com/optimizely/swift-sdk/runs/6074478472?check_suite_focus=true
NOTE
master
before merging in the following places.first, second, third, fourth