feat: enable ci for windows and macos #907
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -19,14 +19,25 @@ on: | |||||
|
|
||||||
| jobs: | ||||||
| build: | ||||||
| env: | ||||||
| BUILD_ARGS: ${{ matrix.os_build_args }} | ||||||
| strategy: | ||||||
| # Run all jobs | ||||||
| fail-fast: false | ||||||
| matrix: | ||||||
| java: [11, 17] | ||||||
| os: [ubuntu-latest, windows-latest, macos-latest] | ||||||
| include: | ||||||
| - os: windows-latest | ||||||
| os_build_args: -x doctest -x integTest -x jacocoTestReport -x compileJdbc | ||||||
|
There was a problem hiding this comment. Disabled doctest/integTest for now, created an issue to enable them in the future |
||||||
| - os: macos-latest | ||||||
| os_build_args: -x doctest -x integTest -x jacocoTestReport -x compileJdbc | ||||||
| runs-on: ${{ matrix.os }} | ||||||
|
|
||||||
| steps: | ||||||
| - name: Change line endings for windows | ||||||
| if: ${{ matrix.os == 'windows-latest' }} | ||||||
| run: git config --global core.autocrlf input | ||||||
joshuali925 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| - uses: actions/checkout@v3 | ||||||
|
|
||||||
| - name: Set up JDK ${{ matrix.java }} | ||||||
|
|
@@ -36,9 +47,10 @@ jobs: | |||||
| java-version: ${{ matrix.java }} | ||||||
|
|
||||||
| - name: Build with Gradle | ||||||
| run: ./gradlew --continue build ${{ env.BUILD_ARGS }} | ||||||
|
There was a problem hiding this comment. Is assemble unnecessary here? It seems build includes assemble: There was a problem hiding this comment. I think you can use there
Suggested change
There was a problem hiding this comment. You're right this is cleaner let me do this |
||||||
|
|
||||||
| - name: Run backward compatibility tests | ||||||
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||||||
|
There was a problem hiding this comment. Does this not work on other platforms? There was a problem hiding this comment. No, this is blocked on opensearch-project/opensearch-plugins#95 (comment). I'm not sure if we should block this PR on that decision @dblock There was a problem hiding this comment. @dblock could this be configured in an extended matrix.entry list? Each jvm+os also carries flags on wether it should upload, run-backward-compat-test, etc? |
||||||
| run: ./scripts/bwctest.sh | ||||||
|
|
||||||
| - name: Create Artifact Path | ||||||
|
|
@@ -48,7 +60,7 @@ jobs: | |||||
|
|
||||||
| # This step uses the codecov-action Github action: https://github.com/codecov/codecov-action | ||||||
| - name: Upload SQL Coverage Report | ||||||
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||||||
Yury-Fridlyand marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| uses: codecov/codecov-action@v3 | ||||||
| with: | ||||||
| flags: sql-engine | ||||||
|
|
@@ -57,11 +69,11 @@ jobs: | |||||
| - name: Upload Artifacts | ||||||
| uses: actions/upload-artifact@v2 | ||||||
| with: | ||||||
| name: opensearch-sql-${{ matrix.os }} | ||||||
| path: opensearch-sql-builds | ||||||
|
|
||||||
| - name: Upload test reports | ||||||
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||||||
Yury-Fridlyand marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| uses: actions/upload-artifact@v2 | ||||||
| with: | ||||||
| name: test-reports | ||||||
|
|
||||||
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.
There's a cleaner way to do this, e.g. https://github.com/opensearch-project/opensearch-java/blob/main/.github/workflows/test-integration.yml#L11
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.
Personally I think it is cleaner as is, there is less lines to maintain instead of manually enumerating all combinations. Will wait for repo maintainers to review and we can go with whatever they think is best.
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 am +1 to Daniel's comment. I'd rather see the json-looking iterations. Sometime in the future we may want to change which mixes of jvm+os are supported, and it's much easier to pick and choose as time goes on.
For instance, jvm 20 comes out, but it only builds correctly on {linux} for nearly a year.
Do we not test jvm20 at all until it can build on all three os? Or do we supply "proof" of tested environemnt for those environments that we can show?
I favor being able to test what we can when we can.
Thoughts @dblock ?