Skip to content
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

Add windows platform in CI workflow #330

Merged
merged 10 commits into from
Nov 1, 2022
Merged

Add windows platform in CI workflow #330

merged 10 commits into from
Nov 1, 2022

Conversation

jackiehanyang
Copy link
Collaborator

Signed-off-by: Jackie Han [email protected]

Description

Add windows platform in CI workflow

Issues Resolved

#104

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
  • 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.

@jackiehanyang jackiehanyang requested a review from a team October 28, 2022 19:17
@opensearch-trigger-bot opensearch-trigger-bot bot added backport 2.x github actions Updating or adding GitHub actions labels Oct 28, 2022
@ohltyler ohltyler mentioned this pull request Oct 31, 2022
1 task
@ohltyler ohltyler linked an issue Oct 31, 2022 that may be closed by this pull request
2 tasks
@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2022

Codecov Report

Merging #330 (1b7f0b3) into main (536171d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #330   +/-   ##
=======================================
  Coverage   52.04%   52.04%           
=======================================
  Files         147      147           
  Lines        5015     5015           
  Branches      965      965           
=======================================
  Hits         2610     2610           
  Misses       2148     2148           
  Partials      257      257           
Impacted Files Coverage Δ
...es/DefineDetector/components/Settings/Settings.tsx 100.00% <0.00%> (ø)
...s/Overview/containers/AnomalyDetectionOverview.tsx 32.92% <0.00%> (ø)
...mponents/NameAndDescription/NameAndDescription.tsx 100.00% <0.00%> (ø)

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

anomalies. The shorter the interval is, the more real time the
detector results will be, and the more computing resources the
detector will need."
hint={[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in an array now?

Copy link
Collaborator Author

@jackiehanyang jackiehanyang Nov 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise new line will be recognized as actual new line in windows, but not in Ubuntu and macos. Sending you a screenshot offline.

Sorry, cannot find the screenshot anymore. But you can find the snapshot diff in this commit - faef809

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So not sure, but why is this needed in an array and none of the others? I don't see any explicit newline in the text.

Copy link
Collaborator Author

@jackiehanyang jackiehanyang Nov 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original text is in three lines, 41, 42, 43. The actual Return key will somehow create an actual new line in windows. This is not the only place that long text uses an array to contain the hint text. Not sure why this long text wan't using an array when it was firstly added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - I wonder why this only fails for 3 lines vs 2 lines. I see a bunch of places where 2 lines doesn't need an array...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you point me to where 2 lines doesn't need an array? I believe 2 lines will fail with the exact same reason as well, and I have addressed them all in this PR

@ohltyler
Copy link
Member

ohltyler commented Nov 1, 2022

I see this is for unit tests, what about for the integration tests?

@jackiehanyang
Copy link
Collaborator Author

I see this is for unit tests, what about for the integration tests?

the flaky integ tests has been failing since Sep 7, it's a separate issue than adding windows workflow. Let's get this adding windows workflow checked in first.

@ohltyler
Copy link
Member

ohltyler commented Nov 1, 2022

I see this is for unit tests, what about for the integration tests?

the flaky integ tests has been failing since Sep 7, it's a separate issue than adding windows workflow. Let's get this adding windows workflow checked in first.

I understand the flaky part, but from my understanding it's still a requirement for the 2.4 campaign.

I'm ok to treat the flaky test issue separately, but I think we should have the infrastructure set up to run with windows and get all of the non-flaky tests to pass.

@jackiehanyang
Copy link
Collaborator Author

I see this is for unit tests, what about for the integration tests?

the flaky integ tests has been failing since Sep 7, it's a separate issue than adding windows workflow. Let's get this adding windows workflow checked in first.

I understand the flaky part, but from my understanding it's still a requirement for the 2.4 campaign.

I'm ok to treat the flaky test issue separately, but I think we should have the infrastructure set up to run with windows and get all of the non-flaky tests to pass.

Sure, this PR is just for adding windows CI.

@ohltyler ohltyler removed a link to an issue Nov 1, 2022
2 tasks
@ohltyler
Copy link
Member

ohltyler commented Nov 1, 2022

I see this is for unit tests, what about for the integration tests?

the flaky integ tests has been failing since Sep 7, it's a separate issue than adding windows workflow. Let's get this adding windows workflow checked in first.

I understand the flaky part, but from my understanding it's still a requirement for the 2.4 campaign.
I'm ok to treat the flaky test issue separately, but I think we should have the infrastructure set up to run with windows and get all of the non-flaky tests to pass.

Sure, this PR is just for adding windows CI.

The full windows CI is the integration tests though. Can that be added in this PR so we can close out the 2.4 issue?

@jackiehanyang
Copy link
Collaborator Author

jackiehanyang commented Nov 1, 2022

I see this is for unit tests, what about for the integration tests?

the flaky integ tests has been failing since Sep 7, it's a separate issue than adding windows workflow. Let's get this adding windows workflow checked in first.

I understand the flaky part, but from my understanding it's still a requirement for the 2.4 campaign.

I'm ok to treat the flaky test issue separately, but I think we should have the infrastructure set up to run with windows and get all of the non-flaky tests to pass.

Sure, this PR is just for adding windows CI.

The full windows CI is the integration tests though. Can that be added in this PR so we can close out the 2.4 issue?

Checked map team package and dashboard team package. They all only added windows CI for unit tests. Plus, we don't even have integration tests for macOS. I think we are fine here just adding windows CI for unit tests. Let me know if you still have concerns.

@Zhangxunmt Zhangxunmt merged commit d8af147 into opensearch-project:main Nov 1, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 1, 2022
* Add windows platform in CI workflow

Signed-off-by: Jackie Han <[email protected]>

* Adjust text content for buidling on Windows

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

Signed-off-by: Jackie Han <[email protected]>
(cherry picked from commit d8af147)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 1, 2022
* Add windows platform in CI workflow

Signed-off-by: Jackie Han <[email protected]>

* Adjust text content for buidling on Windows

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

Signed-off-by: Jackie Han <[email protected]>
(cherry picked from commit d8af147)
jackiehanyang added a commit that referenced this pull request Nov 1, 2022
* Add windows platform in CI workflow

Signed-off-by: Jackie Han <[email protected]>

* Adjust text content for buidling on Windows

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

Signed-off-by: Jackie Han <[email protected]>
(cherry picked from commit d8af147)

Co-authored-by: Jackie Han <[email protected]>
jackiehanyang added a commit that referenced this pull request Nov 1, 2022
* Add windows platform in CI workflow

Signed-off-by: Jackie Han <[email protected]>

* Adjust text content for buidling on Windows

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

* Update long text message format in FormattedFormRow

Signed-off-by: Jackie Han <[email protected]>

Signed-off-by: Jackie Han <[email protected]>
(cherry picked from commit d8af147)

Co-authored-by: Jackie Han <[email protected]>
@opensearch-trigger-bot
Copy link
Contributor

The backport to 1.3 failed:

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

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-1.3 1.3
# Navigate to the new working tree
cd .worktrees/backport-1.3
# Create a new branch
git switch --create backport/backport-330-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d8af1473501e1beb8d3fdef72dc814cb2040f5f6
# Push it to GitHub
git push --set-upstream origin backport/backport-330-to-1.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3

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

@ohltyler
Copy link
Member

ohltyler commented Dec 5, 2022

Will have a manual backport for this for 1.3 in #361

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants