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

Added notifications-core and notifications to 2.0 and add integTest manifest #1957

Merged
merged 7 commits into from
Apr 13, 2022

Conversation

dblock
Copy link
Member

@dblock dblock commented Apr 12, 2022

Signed-off-by: dblock dblock@dblock.org

Description

Simpler alternative to #1956. Requires opensearch-project/notifications#403

Issues Resolved

Closes #1950.

Check List

  • 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: dblock <dblock@dblock.org>
Signed-off-by: dblock <dblock@dblock.org>
@peterzhuamazon
Copy link
Member

As discussed in #1956 we will go with #1957 for now.
This allows us to use existing code base and functionalities to resolve the problem without changing too much for this specific use case.

Thanks.

peterzhuamazon and others added 2 commits April 12, 2022 22:15
Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
Signed-off-by: dblock <dblock@dblock.org>
@dblock
Copy link
Member Author

dblock commented Apr 12, 2022

CI was failing on checks because it tries to execute ./gradlew in notiications/core which doesn't exist. I just removed the checks for now. Really notifications/core feels like a very weird hack all around.

@codecov-commenter
Copy link

codecov-commenter commented Apr 12, 2022

Codecov Report

Merging #1957 (a362984) into main (8522a31) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1957   +/-   ##
=========================================
  Coverage     94.46%   94.46%           
  Complexity       22       22           
=========================================
  Files           179      179           
  Lines          3649     3649           
  Branches         29       29           
=========================================
  Hits           3447     3447           
  Misses          196      196           
  Partials          6        6           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8522a31...a362984. Read the comment docs.

@peterzhuamazon
Copy link
Member

I have tweaked the build.sh so it will build correctly now.

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
@peterzhuamazon
Copy link
Member

This will go in with another PR: opensearch-project/notifications#404

@peterzhuamazon
Copy link
Member

Seems like notifications due to name field is not matching the repo name of notifications.
It is better to save the build scripts in build repo not notifications repo.

@dblock
Copy link
Member Author

dblock commented Apr 13, 2022

Seems like notifications due to name field is not matching the repo name of notifications.

I don't think so. The check for notifications-core just failed with gradlew being not found, because there's none in notifications/core. Build works because we have notifications/core/scripts.

It is better to save the build scripts in build repo not notifications repo.

Build scripts change version-to-version, so they need to be different and follow notifications branching. If you leave them in opensearch-build with these changes, 1.3.x will start failing.

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Block for a bit as we are still fixing the build scripts.
Due to the double subfolder structure of notifications the builds folder and plugins are not correctly copied to tar/builds/opensearch/plugins.

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
@peterzhuamazon peterzhuamazon changed the title Added notifications-core and notifications to 2.0. Added notifications-core and notifications to 2.0 and add integTest manifest Apr 13, 2022
@peterzhuamazon peterzhuamazon dismissed their stale review April 13, 2022 01:38

We have fixed the issues for now and build script can correctly find the builds folder.

@gaiksaya
Copy link
Member

@peterzhuamazon Can we add the checks back or are they still failing?

@peterzhuamazon
Copy link
Member

@peterzhuamazon Can we add the checks back or are they still failing?

They will not pass as notifications structure is different from any other repos.

They need to build within notifications folder, but working directory is pointing to notifications/core and notifications/notifications and those two folders does not have gradle wrapper to run the tests.

@peterzhuamazon peterzhuamazon merged commit c9b3a51 into opensearch-project:main Apr 13, 2022
@peterzhuamazon
Copy link
Member

@gaiksaya fixed in #1959

@dblock dblock deleted the notifications-core branch April 13, 2022 12:27
prudhvigodithi pushed a commit to prudhvigodithi/opensearch-build that referenced this pull request Apr 13, 2022
…arch-project#1947)

Signed-off-by: Sagar Upadhyaya <sagar.upadhyaya.121@gmail.com>

Adding code block for maven plugin zips

Signed-off-by: pgodithi <pgodithi@amazon.com>

Adding code block for maven plugin zips

Signed-off-by: pgodithi <pgodithi@amazon.com>

zip maven publish

Signed-off-by: pgodithi <pgodithi@amazon.com>

added zipsmaven dir check

Signed-off-by: pgodithi <pgodithi@amazon.com>

Adding better gradle project dir

Signed-off-by: pgodithi <pgodithi@amazon.com>

test maven snapshots

Signed-off-by: pgodithi <pgodithi@amazon.com>

test maven snapshots

Signed-off-by: pgodithi <pgodithi@amazon.com>

test maven snapshots

Signed-off-by: pgodithi <pgodithi@amazon.com>

test maven snapshots

Signed-off-by: pgodithi <pgodithi@amazon.com>

test maven snapshots

Signed-off-by: pgodithi <pgodithi@amazon.com>

test maven snapshots

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

Removing build script for PA (opensearch-project#1952)

Signed-off-by: Sagar Upadhyaya <sagar.upadhyaya.121@gmail.com>

update release issue with steps to update ansible and helm (opensearch-project#1942)

* update release issue with steps to update ansible and helm

Signed-off-by: Abhinav Gupta <abhng@amazon.com>

* updated test as sample PR

Signed-off-by: Abhinav Gupta <abhng@amazon.com>

* adding periods

Signed-off-by: Abhinav Gupta <abhng@amazon.com>

Fix the path for maven sign and staging job (opensearch-project#1954)

* Fix the path for staging maven

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

* Add the distribution folder tar to the artifact path and update test

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

[OSD][2.0.0] use rc1 qualifier (opensearch-project#1949)

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

install createrepo in docker image for RPM yum repo support (opensearch-project#1955)

* install createrepo in docker image for RPM yum repo support

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update opensearch to use new image

Signed-off-by: Tianle Huang <tianleh@amazon.com>

Add systemd docker for rpm service test (opensearch-project#1958)

* Add systemd based image for yum install testing and more

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* More packages to add to the images

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

lower freq for build for 1.3.1 opensearch-dashboards (opensearch-project#1941)

* lower freq for build for 1.3.1 opensearch-dashboards

Signed-off-by: Abhinav Gupta <abhng@amazon.com>

* removed build for 1.2.x and 1.3.1

Signed-off-by: Abhinav Gupta <abhng@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

Added notifications-core and notifications to 2.0 and add integTest manifest (opensearch-project#1957)

* Added notifications-core and notifications to 2.0.

Signed-off-by: dblock <dblock@dblock.org>

* Remove scripts from opensearch-build.

Signed-off-by: dblock <dblock@dblock.org>

* Remove dashboards notifications scripts to use the default one

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Remove CI checks for lack of gradlew.

Signed-off-by: dblock <dblock@dblock.org>

* Fix the second subfolder location after core

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Add notifications related build scripts to build repo

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Tweak build scripts to correctly move the zips

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

Co-authored-by: Peter Zhu <zhujiaxi@amazon.com>

Make sure notifications is built correctly while passing ci checks (opensearch-project#1959)

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>

test snapshot build

Signed-off-by: pgodithi <pgodithi@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supporting installation of multiple artifacts for plugin components (related to Notifications)
5 participants