[Backport 2.x] Add CI for Windows and MacOS platforms (#2190) #2205
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add CI for Windows and MacOS platforms Due to the increase in the number of platforms, I've separated the newer integration tests into their own workflow. Until retries have been enabled they will automatically pass - but still run and report logs. As soon as we have full confidence we will allow them to start blocking pull requests from merging. #2184 Switch the gradle commands to be platform agnostic via the `gradle/gradle-build-action@v2`, dropping the 'clean' step to the build which allows us to reuse the gradle cache - if we see any problems pulling in more recent snapshots we can disable this setting quickly. Found and fixed an issued with config value replacement via environment variables, long story short Windows and MacOS allow for many more characters that are used in the unix environment variable landscape. Added new tests to cover these interesting scenarios as well. Found an encoding issue with user names from config files, still unclear of the source of this issue, be it test setup specific or a problem in the broader OpenSearch ecosystem, disabling the `testSpecialUsernames` until we can dive deeper. #2194 Disabled the HeapBasedRateTrackerTests - it was depending on system timing and was very brittle if the system under test experienced any undo load, created follow up issue #2193 Fixed a test issue in testDlsWithMinDocCountZeroAggregations where there was a random chance for a test failure, easier to find intermittent tests when they are run so often. OpenSSL has open questions - while it is supported for our Linux JDK11 builds, it seems like a stopgap measure. I've disabled the tests on windows environment while we determine if we should support OpenSSL at all on Windows JDK11. #2195 Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit a57fd0a)
peternied
force-pushed
the
backport/backport-2190-to-2.x
branch
from
d0acd61 to
385f5bc
Compare
Codecov Report
@@ Coverage Diff @@
## 2.x #2205 +/- ##
============================================
- Coverage 61.06% 61.03% -0.04%
- Complexity 3232 3240 +8
============================================
Files 257 257
Lines 18089 18089
Branches 3225 3225
============================================
- Hits 11046 11040 -6
- Misses 5467 5480 +13
+ Partials 1576 1569 -7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
3 tasks
| strategy: | ||
| fail-fast: true | ||
| fail-fast: false |
There was a problem hiding this comment.
Why do we not want to fail-fast anymore? I believe this will now let the whole matrix operate even if one of the builds fails for a specific OS.
There was a problem hiding this comment.
Fast failing helps free resources faster, but you build less confidence per build iteration. By forcing all builds to keep running we are sure that all integration test runs are completed and you can compare different results between different platforms. If we were optimizing for cost we might stick to the fast failures
With the platform differences, bugs can be introduced into two platforms at once impacting different tests, IMO its more useful for troubleshooting to be able to see if only one workflow was impacted (indicating a random failure) vs many workflows (indicating a new bug was caught).
| path: | | ||
| ./build/reports/ | ||
|
|
||
| - name: check archive for debugging | ||
| if: always() | ||
| run: echo "Check the artifact ${{ matrix.jdk }}-reports.zip for detailed test results" | ||
| run: echo "Check the artifact ${{ matrix.platform }}-JDK${{ matrix.jdk }}-reports for detailed test results" |
There was a problem hiding this comment.
Nice use of all the matrices 👍
| private static final Pattern ENV_PATTERN = Pattern.compile("\\$\\{env\\.([\\w]+)((\\:\\-)?[\\w]*)\\}"); | ||
| private static final Pattern ENVBC_PATTERN = Pattern.compile("\\$\\{envbc\\.([\\w]+)((\\:\\-)?[\\w]*)\\}"); | ||
| private static final Pattern ENVBASE64_PATTERN = Pattern.compile("\\$\\{envbase64\\.([\\w]+)((\\:\\-)?[\\w]*)\\}"); | ||
| private static final String ENV_PATTERN_SUFFIX = "\\.([\\w=():\\-_]+?)(\\:\\-[\\w=():\\-_]*)?\\}"; |
There was a problem hiding this comment.
I know this was already here but maybe we could add a comment saying what this pattern means for readability?
There was a problem hiding this comment.
This is a backport PR, I'll create a separate small PR to update the comments around this file to make it easier to understand
| @@ -269,6 +270,7 @@ public void testSingle() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| @Ignore // https://github.com/opensearch-project/security/issues/2194 | |||
There was a problem hiding this comment.
I think you remove this test in your next PR haha.
| @@ -366,7 +366,7 @@ public void testDlsWithMinDocCountZeroAggregations() throws Exception { | |||
|
|
|||
| // Significant Text Aggregation is not impacted. | |||
| // Non-admin user with setting "min_doc_count=0". Expected to only have access to buckets for dept_manager". | |||
| String query3 = "{\"aggregations\":{\"significant_termX\":{\"significant_terms\":{\"field\":\"termX.keyword\",\"min_doc_count\":0}}}}"; | |||
| String query3 = "{\"size\":100,\"aggregations\":{\"significant_termX\":{\"significant_terms\":{\"field\":\"termX.keyword\",\"min_doc_count\":0}}}}"; | |||
There was a problem hiding this comment.
What was the reason for swapping to a hard coded implementation?
There was a problem hiding this comment.
This test inserts 17 documents and then does a query for matches on them checking the 'hits'. This hits have a default limit of 10... so this test was failing all the time because it depends if you got the right mix of 10/17 items in the output. By making this size much larger this should prevent this issue from happening again.
Always be careful with defaults!
Originally posted by @peternied in #2190 (comment)
stephen-crawford
pushed a commit
to stephen-crawford/security
that referenced
this pull request
Nov 10, 2022
…nsearch-project#2205) Add CI for Windows and MacOS platforms Due to the increase in the number of platforms, I've separated the newer integration tests into their own workflow. Until retries have been enabled they will automatically pass - but still run and report logs. As soon as we have full confidence we will allow them to start blocking pull requests from merging. opensearch-project#2184 Switch the gradle commands to be platform agnostic via the `gradle/gradle-build-action@v2`, dropping the 'clean' step to the build which allows us to reuse the gradle cache - if we see any problems pulling in more recent snapshots we can disable this setting quickly. Found and fixed an issued with config value replacement via environment variables, long story short Windows and MacOS allow for many more characters that are used in the unix environment variable landscape. Added new tests to cover these interesting scenarios as well. Found an encoding issue with user names from config files, still unclear of the source of this issue, be it test setup specific or a problem in the broader OpenSearch ecosystem, disabling the `testSpecialUsernames` until we can dive deeper. opensearch-project#2194 Disabled the HeapBasedRateTrackerTests - it was depending on system timing and was very brittle if the system under test experienced any undo load, created follow up issue opensearch-project#2193 Fixed a test issue in testDlsWithMinDocCountZeroAggregations where there was a random chance for a test failure, easier to find intermittent tests when they are run so often. OpenSSL has open questions - while it is supported for our Linux JDK11 builds, it seems like a stopgap measure. I've disabled the tests on windows environment while we determine if we should support OpenSSL at all on Windows JDK11. opensearch-project#2195 Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit a57fd0a)
stephen-crawford
pushed a commit
to stephen-crawford/security
that referenced
this pull request
Nov 10, 2022
…nsearch-project#2205) Add CI for Windows and MacOS platforms Due to the increase in the number of platforms, I've separated the newer integration tests into their own workflow. Until retries have been enabled they will automatically pass - but still run and report logs. As soon as we have full confidence we will allow them to start blocking pull requests from merging. opensearch-project#2184 Switch the gradle commands to be platform agnostic via the `gradle/gradle-build-action@v2`, dropping the 'clean' step to the build which allows us to reuse the gradle cache - if we see any problems pulling in more recent snapshots we can disable this setting quickly. Found and fixed an issued with config value replacement via environment variables, long story short Windows and MacOS allow for many more characters that are used in the unix environment variable landscape. Added new tests to cover these interesting scenarios as well. Found an encoding issue with user names from config files, still unclear of the source of this issue, be it test setup specific or a problem in the broader OpenSearch ecosystem, disabling the `testSpecialUsernames` until we can dive deeper. opensearch-project#2194 Disabled the HeapBasedRateTrackerTests - it was depending on system timing and was very brittle if the system under test experienced any undo load, created follow up issue opensearch-project#2193 Fixed a test issue in testDlsWithMinDocCountZeroAggregations where there was a random chance for a test failure, easier to find intermittent tests when they are run so often. OpenSSL has open questions - while it is supported for our Linux JDK11 builds, it seems like a stopgap measure. I've disabled the tests on windows environment while we determine if we should support OpenSSL at all on Windows JDK11. opensearch-project#2195 Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit a57fd0a) Signed-off-by: Stephen Crawford <steecraw@amazon.com>
stephen-crawford
pushed a commit
to stephen-crawford/security
that referenced
this pull request
Nov 10, 2022
…nsearch-project#2205) Add CI for Windows and MacOS platforms Due to the increase in the number of platforms, I've separated the newer integration tests into their own workflow. Until retries have been enabled they will automatically pass - but still run and report logs. As soon as we have full confidence we will allow them to start blocking pull requests from merging. opensearch-project#2184 Switch the gradle commands to be platform agnostic via the `gradle/gradle-build-action@v2`, dropping the 'clean' step to the build which allows us to reuse the gradle cache - if we see any problems pulling in more recent snapshots we can disable this setting quickly. Found and fixed an issued with config value replacement via environment variables, long story short Windows and MacOS allow for many more characters that are used in the unix environment variable landscape. Added new tests to cover these interesting scenarios as well. Found an encoding issue with user names from config files, still unclear of the source of this issue, be it test setup specific or a problem in the broader OpenSearch ecosystem, disabling the `testSpecialUsernames` until we can dive deeper. opensearch-project#2194 Disabled the HeapBasedRateTrackerTests - it was depending on system timing and was very brittle if the system under test experienced any undo load, created follow up issue opensearch-project#2193 Fixed a test issue in testDlsWithMinDocCountZeroAggregations where there was a random chance for a test failure, easier to find intermittent tests when they are run so often. OpenSSL has open questions - while it is supported for our Linux JDK11 builds, it seems like a stopgap measure. I've disabled the tests on windows environment while we determine if we should support OpenSSL at all on Windows JDK11. opensearch-project#2195 Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit a57fd0a) Signed-off-by: Stephen Crawford <steecraw@amazon.com>
wuychn
pushed a commit
to ochprince/security
that referenced
this pull request
Mar 16, 2023
…nsearch-project#2205) Add CI for Windows and MacOS platforms Due to the increase in the number of platforms, I've separated the newer integration tests into their own workflow. Until retries have been enabled they will automatically pass - but still run and report logs. As soon as we have full confidence we will allow them to start blocking pull requests from merging. opensearch-project#2184 Switch the gradle commands to be platform agnostic via the `gradle/gradle-build-action@v2`, dropping the 'clean' step to the build which allows us to reuse the gradle cache - if we see any problems pulling in more recent snapshots we can disable this setting quickly. Found and fixed an issued with config value replacement via environment variables, long story short Windows and MacOS allow for many more characters that are used in the unix environment variable landscape. Added new tests to cover these interesting scenarios as well. Found an encoding issue with user names from config files, still unclear of the source of this issue, be it test setup specific or a problem in the broader OpenSearch ecosystem, disabling the `testSpecialUsernames` until we can dive deeper. opensearch-project#2194 Disabled the HeapBasedRateTrackerTests - it was depending on system timing and was very brittle if the system under test experienced any undo load, created follow up issue opensearch-project#2193 Fixed a test issue in testDlsWithMinDocCountZeroAggregations where there was a random chance for a test failure, easier to find intermittent tests when they are run so often. OpenSSL has open questions - while it is supported for our Linux JDK11 builds, it seems like a stopgap measure. I've disabled the tests on windows environment while we determine if we should support OpenSSL at all on Windows JDK11. opensearch-project#2195 Signed-off-by: Peter Nied <petern@amazon.com> (cherry picked from commit a57fd0a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
[Backport 2.x] Add CI for Windows and MacOS platforms
Due to the increase in the number of platforms, I've separated the newer integration tests into their own workflow. Until retries have been enabled they will automatically pass - but still run and report logs. As soon as we have full confidence we will allow them to start blocking pull requests from merging. #2184
Switch the gradle commands to be platform agnostic via the
gradle/gradle-build-action@v2, dropping the 'clean' step to the build which allows us to reuse the gradle cache - if we see any problems pulling in more recent snapshots we can disable this setting quickly.Found and fixed an issued with config value replacement via environment variables, long story short Windows and MacOS allow for many more characters that are used in the unix environment variable landscape. Added new tests to cover these interesting scenarios as well.
Found an encoding issue with user names from config files, still unclear of the source of this issue, be it test setup specific or a problem in the broader OpenSearch ecosystem, disabling the
testSpecialUsernamesuntil we can dive deeper. #2194Disabled the HeapBasedRateTrackerTests - it was depending on system timing and was very brittle if the system under test experienced any undo load, created follow up issue #2193
Fixed a test issue in testDlsWithMinDocCountZeroAggregations where there was a random chance for a test failure, easier to find intermittent tests when they are run so often.
OpenSSL has open questions - while it is supported for our Linux JDK11 builds, it seems like a stopgap measure. I've disabled the tests on windows environment while we determine if we should support OpenSSL at all on Windows JDK11. #2195
Signed-off-by: Peter Nied petern@amazon.com
(cherry picked from commit a57fd0a)
Check List
New functionality has been documentedBy 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.