feat: enable ci for windows and macos #907

derek-ho · 2022-10-12T19:25:15Z

Signed-off-by: Derek Ho dxho@amazon.com

Description

[Describe what this change achieves]

Issues Resolved

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Derek Ho <dxho@amazon.com>

codecov-commenter · 2022-10-12T19:35:35Z

Codecov Report

Merging #907 (79f765c) into 2.x (057fa44) will decrease coverage by 35.10%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##                2.x     #907       +/-   ##
=============================================
- Coverage     97.87%   62.76%   -35.11%     
=============================================
  Files           284       10      -274     
  Lines          7425      658     -6767     
  Branches        475      119      -356     
=============================================
- Hits           7267      413     -6854     
- Misses          157      192       +35     
- Partials          1       53       +52

Flag	Coverage Δ
query-workbench	`62.76% <ø> (?)`
sql-engine	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...l/planner/optimizer/rule/MergeFilterAndFilter.java
...nsearch/sql/storage/bindingtuple/BindingTuple.java
.../org/opensearch/sql/planner/logical/LogicalAD.java
...ch/sql/planner/logical/LogicalPlanNodeVisitor.java
.../opensearch/sql/opensearch/executor/Scheduler.java
...g/opensearch/sql/expression/text/TextFunction.java
...org/opensearch/sql/expression/NamedExpression.java
...arch/sql/opensearch/response/agg/FilterParser.java
...earch/sql/opensearch/data/utils/ObjectContent.java
...search/sql/opensearch/security/SecurityAccess.java
... and 284 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Yury-Fridlyand

You can use a matrix build to run a workflow on few OS.
https://devblogs.microsoft.com/premier-developer/github-actions-overview/#matrix-builds

Signed-off-by: Derek Ho <dxho@amazon.com>

derek-ho · 2022-10-17T14:38:37Z

.github/workflows/sql-test-and-build-workflow.yml

+        os: [ubuntu-latest, windows-latest, macos-latest]
+        include:
+          - os: windows-latest
+            os_build_args: -x doctest -x integTest -x jacocoTestReport -x compileJdbc


Disabled doctest/integTest for now, created an issue to enable them in the future
#921

derek-ho · 2022-10-17T14:40:05Z

.github/workflows/sql-test-and-build-workflow.yml

@@ -36,9 +47,10 @@ jobs:
        java-version: ${{ matrix.java }}

    - name: Build with Gradle
-      run: ./gradlew --continue build assemble
+      run: ./gradlew --continue build ${{ env.BUILD_ARGS }}


Is assemble unnecessary here? It seems build includes assemble:
https://stackoverflow.com/a/44185464

protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java

workbench/package.json

Signed-off-by: Derek Ho <dxho@amazon.com>

Yury-Fridlyand · 2022-10-17T18:39:07Z

protocol/src/test/java/org/opensearch/sql/protocol/response/format/ErrorFormatterTest.java

@@ -16,7 +17,7 @@ class ErrorFormatterTest {
  @Test
  void htmlEscaping_should_disabled() {
    assertEquals(
-        "{\n" + "  \"request\": \"index=test\"\n" + "}",
+        format("{\n" + "  \"request\": \"index=test\"\n" + "}"),


Should be there %n too?

Good catch I reverted this file. As per @joshuali925's suggestion, I will not be doing replaceAll in ErrorFormatter, as this could affect user entered input. Reverted this file to initial state, as I am no longer doing anything in the ErrorFormatter itself.

Signed-off-by: Derek Ho <dxho@amazon.com>

.github/workflows/sql-test-and-build-workflow.yml

derek-ho · 2022-10-19T14:35:00Z

Looks like at least integ tests are disabled on Windows. That's fine, but please open an issue to re-enable them.

Done: #921

derek-ho · 2022-10-19T14:47:03Z

@Yury-Fridlyand can you give it another review and remove the changes requested block if things look good?

.github/workflows/sql-test-and-build-workflow.yml

.github/workflows/sql-workbench-test-and-build-workflow.yml

…s succeeds Signed-off-by: Derek Ho <dxho@amazon.com>

Signed-off-by: Derek Ho <dxho@amazon.com>

Yury-Fridlyand · 2022-10-19T21:40:39Z

Why ubuntu CI goes to download artifacts for macos?
https://github.com/opensearch-project/sql/actions/runs/3285095735/jobs/5411815462

> Could not resolve all files for configuration ':integ-test:opensearch_distro_extracted_testclusters-integ-test-sqlBwcCluster1-0-1.1.0-'.

   > Could not resolve opensearch-distribution:opensearch:1.1.0.
     Required by:
         project :integ-test
      > Could not resolve opensearch-distribution:opensearch:1.1.0.
         > Could not get resource 'https://artifacts.opensearch.org/releases/core/opensearch/1.1.0/opensearch-min-1.1.0-darwin-x64.tar.gz'.
            > Could not HEAD 'https://artifacts.opensearch.org/releases/core/opensearch/1.1.0/opensearch-min-1.1.0-darwin-x64.tar.gz'. Received status code 403 from server: Forbidden
      > Could not resolve opensearch-distribution:opensearch:1.1.0.
         > Could not get resource 'https://artifacts.opensearch.org/release-candidates/core/opensearch/1.1.0/opensearch-min-1.1.0-darwin-x64.tar.gz'.
            > Could not HEAD 'https://artifacts.opensearch.org/release-candidates/core/opensearch/1.1.0/opensearch-min-1.1.0-darwin-x64.tar.gz'. Received status code 403 from server: Forbidden

derek-ho · 2022-10-19T22:36:31Z

Why ubuntu CI goes to download artifacts for macos? https://github.com/opensearch-project/sql/actions/runs/3285095735/jobs/5411815462

> Could not resolve all files for configuration ':integ-test:opensearch_distro_extracted_testclusters-integ-test-sqlBwcCluster1-0-1.1.0-'.

   > Could not resolve opensearch-distribution:opensearch:1.1.0.
     Required by:
         project :integ-test
      > Could not resolve opensearch-distribution:opensearch:1.1.0.
         > Could not get resource 'https://artifacts.opensearch.org/releases/core/opensearch/1.1.0/opensearch-min-1.1.0-darwin-x64.tar.gz'.
            > Could not HEAD 'https://artifacts.opensearch.org/releases/core/opensearch/1.1.0/opensearch-min-1.1.0-darwin-x64.tar.gz'. Received status code 403 from server: Forbidden
      > Could not resolve opensearch-distribution:opensearch:1.1.0.
         > Could not get resource 'https://artifacts.opensearch.org/release-candidates/core/opensearch/1.1.0/opensearch-min-1.1.0-darwin-x64.tar.gz'.
            > Could not HEAD 'https://artifacts.opensearch.org/release-candidates/core/opensearch/1.1.0/opensearch-min-1.1.0-darwin-x64.tar.gz'. Received status code 403 from server: Forbidden

I'm not quite sure I didn't touch that section, but i'll check...

Signed-off-by: Derek Ho <dxho@amazon.com>

derek-ho · 2022-10-20T21:14:55Z

@Yury-Fridlyand Fixed it, please review again, thanks!

Yury-Fridlyand · 2022-10-20T21:22:16Z

.github/workflows/sql-test-and-build-workflow.yml

    - name: Build with Gradle
-      run: ./gradlew --continue build assemble
+      run: ./gradlew --continue build ${{ env.BUILD_ARGS }}


I think you can use there

Suggested change

run: ./gradlew --continue build ${{ env.BUILD_ARGS }}

run: ./gradlew --continue build ${{ matrix.entry.os_build_args }}

You're right this is cleaner let me do this

Yury-Fridlyand · 2022-10-20T21:26:36Z

...ocol/src/test/java/org/opensearch/sql/protocol/response/format/CsvResponseFormatterTest.java

        + "\"John,Smith\",\"30,,,\"";
-    assertEquals(expected, formatter.format(response));
+    assertEquals(format(expected), formatter.format(response));
  }

  @Test


Do you need %n in this test as well?

Yea, put a %n on line 88 in this file

Signed-off-by: Derek Ho <dxho@amazon.com>

legacy/src/test/java/org/opensearch/sql/legacy/unittest/utils/PrettyFormatterTest.java

opensearch-trigger-bot · 2022-10-25T18:01:01Z

The backport to main failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-main main
# Navigate to the new working tree
cd .worktrees/backport-main
# Create a new branch
git switch --create backport/backport-907-to-main
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0699baa2ae47fb72af9721986df669e786655cd2
# Push it to GitHub
git push --set-upstream origin backport/backport-907-to-main
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-main

Then, create a pull request where the base branch is main and the compare/head branch is backport/backport-907-to-main.

…s-macos feat: enable ci for windows and macos (cherry picked from commit 0699baa)

…s-macos feat: enable ci for windows and macos (cherry picked from commit 0699baa) Signed-off-by: Derek Ho <dxho@amazon.com>

…nch (#962) * Merge pull request #907 from derek-ho/enable-windows-macos feat: enable ci for windows and macos (cherry picked from commit 0699baa) Signed-off-by: Derek Ho <dxho@amazon.com> * always and os==ubuntu Signed-off-by: Derek Ho <dxho@amazon.com> Signed-off-by: Derek Ho <dxho@amazon.com>

initial commit for sql workbench windows and macos

2661232

Signed-off-by: Derek Ho <dxho@amazon.com>

derek-ho requested a review from a team as a code owner October 12, 2022 19:25

derek-ho marked this pull request as draft October 12, 2022 19:25

name files

7a1c7ae

Signed-off-by: Derek Ho <dxho@amazon.com>

Yury-Fridlyand requested changes Oct 12, 2022

View reviewed changes

derek-ho added 11 commits October 13, 2022 11:03

clean up code with matrix

ef535e9

Signed-off-by: Derek Ho <dxho@amazon.com>

forgot comma

8a58eb9

Signed-off-by: Derek Ho <dxho@amazon.com>

use cross env for windows and enable windows and macos ci in sql

828101c

Signed-off-by: Derek Ho <dxho@amazon.com>

disable integration and jacoco for windows and mac

53ef534

Signed-off-by: Derek Ho <dxho@amazon.com>

add gitattributes file to normalize line endings

7569d6d

Signed-off-by: Derek Ho <dxho@amazon.com>

move gitattributes into appropriate folder

912731b

Signed-off-by: Derek Ho <dxho@amazon.com>

configure line endings for windows

6a6cfd6

Signed-off-by: Derek Ho <dxho@amazon.com>

fix one test file and get rid of git attributes

0d016ef

Signed-off-by: Derek Ho <dxho@amazon.com>

disable doctest and integ test

a8709d2

Signed-off-by: Derek Ho <dxho@amazon.com>

fix up tests

d4c9f1e

Signed-off-by: Derek Ho <dxho@amazon.com>

fix tests and add java docs

9df2acd

Signed-off-by: Derek Ho <dxho@amazon.com>

derek-ho marked this pull request as ready for review October 17, 2022 14:36

derek-ho commented Oct 17, 2022

View reviewed changes

protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java Outdated Show resolved Hide resolved

derek-ho commented Oct 17, 2022

View reviewed changes

workbench/package.json Outdated Show resolved Hide resolved

Yury-Fridlyand self-requested a review October 17, 2022 16:51

derek-ho added 2 commits October 17, 2022 13:53

revert error format

8f98fa4

Signed-off-by: Derek Ho <dxho@amazon.com>

revert pretty format response

7a39dcb

Signed-off-by: Derek Ho <dxho@amazon.com>

This was referenced Oct 17, 2022

Add Windows Support #249

Closed

Add MacOS Support #250

Closed

Yury-Fridlyand reviewed Oct 17, 2022

View reviewed changes

revert error formatter test file

bd8572a

Signed-off-by: Derek Ho <dxho@amazon.com>

joshuali925 reviewed Oct 17, 2022

View reviewed changes

.github/workflows/sql-test-and-build-workflow.yml Outdated Show resolved Hide resolved

Yury-Fridlyand requested changes Oct 19, 2022

View reviewed changes

.github/workflows/sql-test-and-build-workflow.yml Outdated Show resolved Hide resolved

.github/workflows/sql-test-and-build-workflow.yml Outdated Show resolved Hide resolved

.github/workflows/sql-workbench-test-and-build-workflow.yml Outdated Show resolved Hide resolved

derek-ho added 3 commits October 19, 2022 17:08

fix PR comments and fail test on purpose to see if upload test report…

e70df11

…s succeeds Signed-off-by: Derek Ho <dxho@amazon.com>

fix matrix entry

de65184

Signed-off-by: Derek Ho <dxho@amazon.com>

remove test failure

a6f3ceb

Signed-off-by: Derek Ho <dxho@amazon.com>

Yury-Fridlyand requested review from Yury-Fridlyand and joshuali925 October 19, 2022 21:40

derek-ho added 2 commits October 20, 2022 16:50

fix up to run on right os

3f372bb

Signed-off-by: Derek Ho <dxho@amazon.com>

fix up indentation

5e24f3c

Signed-off-by: Derek Ho <dxho@amazon.com>

Yury-Fridlyand previously approved these changes Oct 20, 2022

View reviewed changes

remove env. build_args to make it cleaner

3fbe3e4

Signed-off-by: Derek Ho <dxho@amazon.com>

derek-ho dismissed Yury-Fridlyand’s stale review via 3fbe3e4 October 20, 2022 21:35

try removing env var

954c072

Signed-off-by: Derek Ho <dxho@amazon.com>

derek-ho force-pushed the enable-windows-macos branch from fb50655 to 954c072 Compare October 20, 2022 21:48

remove unecessary import

79f765c

Signed-off-by: Derek Ho <dxho@amazon.com>

joshuali925 reviewed Oct 20, 2022

View reviewed changes

legacy/src/test/java/org/opensearch/sql/legacy/unittest/utils/PrettyFormatterTest.java Show resolved Hide resolved

joshuali925 approved these changes Oct 25, 2022

View reviewed changes

joshuali925 added the backport main label Oct 25, 2022

Yury-Fridlyand approved these changes Oct 25, 2022

View reviewed changes

derek-ho merged commit 0699baa into opensearch-project:2.x Oct 25, 2022

derek-ho deleted the enable-windows-macos branch October 25, 2022 18:00

derek-ho added a commit to derek-ho/sql that referenced this pull request Oct 25, 2022

Merge pull request opensearch-project#907 from derek-ho/enable-window…

e36738a

…s-macos feat: enable ci for windows and macos (cherry picked from commit 0699baa)

penghuo mentioned this pull request Dec 6, 2022

Enable CI for windows #1140

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable ci for windows and macos #907

feat: enable ci for windows and macos #907

derek-ho commented Oct 12, 2022 •

edited

Loading

codecov-commenter commented Oct 12, 2022 •

edited

Loading

Yury-Fridlyand left a comment

derek-ho Oct 17, 2022

derek-ho Oct 17, 2022

Yury-Fridlyand Oct 17, 2022

derek-ho Oct 17, 2022

derek-ho commented Oct 19, 2022

derek-ho commented Oct 19, 2022

Yury-Fridlyand commented Oct 19, 2022

derek-ho commented Oct 19, 2022

derek-ho commented Oct 20, 2022 •

edited

Loading

Yury-Fridlyand Oct 20, 2022

derek-ho Oct 20, 2022

Yury-Fridlyand Oct 20, 2022

derek-ho Oct 20, 2022

opensearch-trigger-bot bot commented Oct 25, 2022

	run: ./gradlew --continue build ${{ env.BUILD_ARGS }}
	run: ./gradlew --continue build ${{ matrix.entry.os_build_args }}

feat: enable ci for windows and macos #907

feat: enable ci for windows and macos #907

Conversation

derek-ho commented Oct 12, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Oct 12, 2022 • edited Loading

Codecov Report

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derek-ho commented Oct 19, 2022

derek-ho commented Oct 19, 2022

Yury-Fridlyand commented Oct 19, 2022

derek-ho commented Oct 19, 2022

derek-ho commented Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Oct 25, 2022

derek-ho commented Oct 12, 2022 •

edited

Loading

codecov-commenter commented Oct 12, 2022 •

edited

Loading

derek-ho commented Oct 20, 2022 •

edited

Loading