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

ci: reduce redundancy by using matrix strategy on Windows and Linux workflows #3514

Merged
merged 5 commits into from
Mar 13, 2023

Conversation

jlabatut
Copy link
Contributor

@jlabatut jlabatut commented Mar 2, 2023

Description

This PR aims to reduce the amount of redundancy in the CI "build & test" workflow by using matrices, instead of duplicating jobs for both Windows and Linux.

Issues Resolved

Closes #2991

Check List

  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@jlabatut jlabatut requested a review from a team as a code owner March 2, 2023 15:23
jlabatut added a commit to jlabatut/OpenSearch-Dashboards that referenced this pull request Mar 2, 2023
Comment on lines -32 to -34
container:
image: docker://opensearchstaging/ci-runner:ci-runner-rockylinux8-opensearch-dashboards-integtest-v2
options: --user 1001
Copy link
Member

Choose a reason for hiding this comment

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

@kavilla do we still need this custom image? or can we just use the default ubuntu-latest image now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

we shouldn't now that we use the script to find the version but i am worried about a few weeks from now when Node 14 is EoLed and chromedriver drops it.

Copy link
Member

Choose a reason for hiding this comment

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

+1 I don't think we necessarily have to use a custom image besides the time when the chromedriver isn't releasing a new version for chrome when github updates their virtual environments or as @AMoo-Miki mentioned.

i'm fine with this. One other benefit of using a custom image but since the github user 1001 isn't added to the image is that if that if we add the user to have access the dependencies the image can already have all the setup dependencies step in it. Like the image has yarn and java.

AMoo-Miki
AMoo-Miki previously approved these changes Mar 2, 2023
Copy link
Collaborator

@AMoo-Miki AMoo-Miki left a comment

Choose a reason for hiding this comment

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

This looks great; thanks.

.github/workflows/build_and_test_workflow.yml Outdated Show resolved Hide resolved
jlabatut added a commit to jlabatut/OpenSearch-Dashboards that referenced this pull request Mar 2, 2023
@kavilla
Copy link
Member

kavilla commented Mar 3, 2023

This is really awesome! I really appreciate @jlabatut. I'm going to let the most recent push run and will add more feedback. I also think we can consider taking this one step further as these don't really need to be gathered in the same file.

@jlabatut
Copy link
Contributor Author

jlabatut commented Mar 3, 2023

I also think we can consider taking this one step further as these don't really need to be gathered in the same file.

@kavilla What jobs precisely should be split into different workflows ?

@ashwin-pc
Copy link
Member

@jlabatut can you rebase and push to the branch again? The CI workflows did not run on your latest changes.

@jlabatut jlabatut force-pushed the update-workflows-matrix branch from e2ca7a9 to 52cd717 Compare March 3, 2023 22:38
@jlabatut jlabatut requested a review from ashwin-pc March 3, 2023 22:38
@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2023

Codecov Report

Merging #3514 (c21cc94) into main (9c3f65b) will increase coverage by 0.04%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3514      +/-   ##
==========================================
+ Coverage   66.40%   66.45%   +0.04%     
==========================================
  Files        3205     3205              
  Lines       61562    61563       +1     
  Branches     9497     9497              
==========================================
+ Hits        40882    40911      +29     
+ Misses      18404    18380      -24     
+ Partials     2276     2272       -4     
Flag Coverage Δ
Linux 66.39% <100.00%> (-0.01%) ⬇️
Windows 66.40% <100.00%> (?)

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

Impacted Files Coverage Δ
packages/osd-opensearch/src/artifact.js 49.07% <100.00%> (+0.31%) ⬆️
...s/osd-optimizer/src/node/node_auto_tranpilation.ts 83.67% <0.00%> (-4.09%) ⬇️
packages/osd-optimizer/src/node/cache.ts 51.31% <0.00%> (-1.32%) ⬇️
...ic/application/models/sense_editor/sense_editor.ts 64.88% <0.00%> (ø)
src/dev/build/lib/config.ts 85.29% <0.00%> (+5.88%) ⬆️
...ges/osd-apm-config-loader/src/config.test.mocks.ts 100.00% <0.00%> (+8.69%) ⬆️
packages/osd-cross-platform/src/path.ts 85.36% <0.00%> (+34.14%) ⬆️
src/setup_node_env/harden/child_process.js 76.92% <0.00%> (+38.46%) ⬆️
src/dev/build/lib/get_build_number.ts 100.00% <0.00%> (+42.85%) ⬆️

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

@jlabatut
Copy link
Contributor Author

jlabatut commented Mar 6, 2023

Anyone would know why the integration tests (job Build and verify on Linux, tests builds a generated plugin into a viable archive and builds a non-semver generated plugin into a viable archive) and the job Run functional tests on Linux (ciGroup7) (timeout) fail ?

@AMoo-Miki
Copy link
Collaborator

Anyone would know why the integration tests (job Build and verify on Linux, tests builds a generated plugin into a viable archive and builds a non-semver generated plugin into a viable archive) and the job Run functional tests on Linux (ciGroup7) (timeout) fail ?

I think Rocky pushed a PR that should fix the outdated caniuse. I will update the PR and monitor.

@ashwin-pc ashwin-pc assigned kavilla and abbyhu2000 and unassigned kavilla Mar 7, 2023
@kavilla
Copy link
Member

kavilla commented Mar 10, 2023

I also think we can consider taking this one step further as these don't really need to be gathered in the same file.

@kavilla What jobs precisely should be split into different workflows ?

I think as is it's already a huge improvement and I appreciate it. But then we can take it to the next step like this: https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/main/.github/workflows/release-e2e-workflow-template.yml.

Like what I'd imagine is the summary would split, the template will have the main stuff. We probably don't need conditionals as we can pass parameters to execute, but the conditionals are fine for now. Then we have a file for Linux and a file for Windows. That way in the future it's even more maintainable.

This is already really great as is. But would you like to take that as a follow-up @jlabatut? Please let me know if you have any questions.

Thank you!

@jlabatut
Copy link
Contributor Author

This is already really great as is. But would you like to take that as a follow-up @jlabatut? Please let me know if you have any questions.

@kavilla, I would be glad to do this improvement, but wouldn't it be better/faster if this PR is merged, and I create a new issue and/or PR for this ?
That way, I will have the contributor badge, which is more convenient if I want to run the workflows without the need of an approval from the reviewers (correct me if I'm wrong).

I'll work on this as soon as you answer 🙂

@kavilla kavilla merged commit 1e127b0 into opensearch-project:main Mar 13, 2023
@kavilla
Copy link
Member

kavilla commented Mar 13, 2023

This is already really great as is. But would you like to take that as a follow-up @jlabatut? Please let me know if you have any questions.

@kavilla, I would be glad to do this improvement, but wouldn't it be better/faster if this PR is merged, and I create a new issue and/or PR for this ? That way, I will have the contributor badge, which is more convenient if I want to run the workflows without the need of an approval from the reviewers (correct me if I'm wrong).

I'll work on this as soon as you answer 🙂

Done! Thank you so much!

@opensearch-trigger-bot
Copy link
Contributor

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

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3514-to-2.x.

@jlabatut jlabatut deleted the update-workflows-matrix branch March 14, 2023 19:00
kavilla pushed a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Mar 14, 2023
…rch-project#3514)

* ci: reduce redundancy by using matrix strategy on Windows and Linux workflows (opensearch-project#3514)
* update CHANGELOG.md (opensearch-project#3514)
* ci: fix overriding matrix for Linux x86 (opensearch-project#3514)
* ci: update os checks in preparation for darwin (opensearch-project#3514)

Signed-off-by: Julian Labatut <[email protected]>
Co-authored-by: Miki <[email protected]>
kavilla added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Mar 14, 2023
…rch-project#3514)

* ci: reduce redundancy by using matrix strategy on Windows and Linux workflows (opensearch-project#3514)
* update CHANGELOG.md (opensearch-project#3514)
* ci: fix overriding matrix for Linux x86 (opensearch-project#3514)
* ci: update os checks in preparation for darwin (opensearch-project#3514)

Signed-off-by: Julian Labatut <[email protected]>
Co-authored-by: Miki <[email protected]>
Co-authored-by: Kawika Avilla <[email protected]>
@opensearch-trigger-bot
Copy link
Contributor

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

Then, create a pull request where the base branch is 1.x and the compare/head branch is backport/backport-3514-to-1.x.

joshuarrrr pushed a commit that referenced this pull request Mar 17, 2023
…3609)

* ci: reduce redundancy by using matrix strategy on Windows and Linux workflows (#3514)
* update CHANGELOG.md (#3514)
* ci: fix overriding matrix for Linux x86 (#3514)
* ci: update os checks in preparation for darwin (#3514)

Signed-off-by: Julian Labatut <[email protected]>
Co-authored-by: Julian Labatut <[email protected]>
Co-authored-by: Miki <[email protected]>
sikhote pushed a commit to sikhote/OpenSearch-Dashboards that referenced this pull request Apr 24, 2023
…rch-project#3514)

* ci: reduce redundancy by using matrix strategy on Windows and Linux workflows (opensearch-project#3514)
* update CHANGELOG.md (opensearch-project#3514)
* ci: fix overriding matrix for Linux x86 (opensearch-project#3514)
* ci: update os checks in preparation for darwin (opensearch-project#3514)

Signed-off-by: Julian Labatut <[email protected]>
Co-authored-by: Miki <[email protected]>
Signed-off-by: David Sinclair <[email protected]>
sikhote pushed a commit to sikhote/OpenSearch-Dashboards that referenced this pull request Apr 24, 2023
…rch-project#3514)

* ci: reduce redundancy by using matrix strategy on Windows and Linux workflows (opensearch-project#3514)
* update CHANGELOG.md (opensearch-project#3514)
* ci: fix overriding matrix for Linux x86 (opensearch-project#3514)
* ci: update os checks in preparation for darwin (opensearch-project#3514)

Signed-off-by: Julian Labatut <[email protected]>
Co-authored-by: Miki <[email protected]>
Signed-off-by: David Sinclair <[email protected]>
@joshuarrrr
Copy link
Member

@kavilla I'm relabeling for 1.3.11, as this was never backported to 1.x

joshuarrrr pushed a commit to joshuarrrr/OpenSearch-Dashboards that referenced this pull request May 12, 2023
…rch-project#3514)

* ci: reduce redundancy by using matrix strategy on Windows and Linux workflows (opensearch-project#3514)
* update CHANGELOG.md (opensearch-project#3514)
* ci: fix overriding matrix for Linux x86 (opensearch-project#3514)
* ci: update os checks in preparation for darwin (opensearch-project#3514)

Signed-off-by: Julian Labatut <[email protected]>
Co-authored-by: Miki <[email protected]>
(cherry picked from commit 1e127b0)
@joshuarrrr
Copy link
Member

We won't be backporting this to the 1.3 line.

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.

[CI] Templates to CI workflows
8 participants