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 build scripts for Notifications and Notifications Dashboards #398

Merged
merged 1 commit into from
Apr 7, 2022

Conversation

qreshi
Copy link
Contributor

@qreshi qreshi commented Apr 7, 2022

Signed-off-by: Mohammad Qureshi 47198598+qreshi@users.noreply.github.com

Description

Adding build scripts for Notifications and Notifications Dashboards. Once these are merged in, we can submit another PR to the opensearch-build repo to remove the build scripts for Notifications there since they take precedence. Finally, after all these changes are complete, we can add Notifications to the 2.0 manifest and start testing if the build passes.

Issues Resolved

[List any issues this PR will resolve]

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: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
@qreshi qreshi requested a review from a team April 7, 2022 16:21
@qreshi
Copy link
Contributor Author

qreshi commented Apr 7, 2022

@peterzhuamazon Would you be able to take a look and confirm the scripts are fine and are in the right directories?

For some context on the backend build.sh script under notifications/scripts/, you'll notice it is moving two zips. That is because the backend has two plugins: Notifications and Notifications-Core. The second is required by the first since it extends it, so we are moving both to the output directory (calling assemble the way that we do will build them both since it's assembling the sub-projects).

@qreshi
Copy link
Contributor Author

qreshi commented Apr 7, 2022

Also regarding the CI, the backend one is failing because common-utils has changes that Notifications doesn't have yet (it's in PR). I believe the frontend CI is failing due to the OpenSearch Dashboard issues (some of the recent commits broke the bootstrapping of our plugin).

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.

Please make sure you have a second PR to remove default scripts from build repo.
Thanks.

@peterzhuamazon
Copy link
Member

@peterzhuamazon Would you be able to take a look and confirm the scripts are fine and are in the right directories?

For some context on the backend build.sh script under notifications/scripts/, you'll notice it is moving two zips. That is because the backend has two plugins: Notifications and Notifications-Core. The second is required by the first since it extends it, so we are moving both to the output directory (calling assemble the way that we do will build them both since it's assembling the sub-projects).

Please explain these:

  1. Will notifications needs to be installed to core as bundle?
  2. If so do we need to install both?
  3. If so can we combine them into one?
  4. If not 1 how do we use it?

Thanks.

@qreshi
Copy link
Contributor Author

qreshi commented Apr 7, 2022

Please explain these:

  1. Will notifications needs to be installed to core as bundle?
  2. If so do we need to install both?
  3. If so can we combine them into one?
  4. If not 1 how do we use it?

Thanks.

  1. The cluster will need to install both opensearch-notfications-core and opensearch-notifications to use the Notifications plugin. Trying to start OpenSearch with only the second one installed will fail.
  2. Yes
  3. They were intentionally split this way because the underlying core implementation can be different, even for the same type of Channel "type" so we cannot combine them without redesigning and losing some flexibility
  4. Refer to 1

@peterzhuamazon
Copy link
Member

Please explain these:

  1. Will notifications needs to be installed to core as bundle?
  2. If so do we need to install both?
  3. If so can we combine them into one?
  4. If not 1 how do we use it?

Thanks.

1. The cluster will need to install both `opensearch-notfications-core` and `opensearch-notifications` to use the Notifications plugin. Trying to start OpenSearch with only the second one installed will fail.

2. Yes

3. They were intentionally split this way because the underlying core implementation can be different, even for the same type of Channel "type" so we cannot combine them without redesigning and losing some flexibility

4. Refer to 1

We need some testing on this as we have never tried to install 2 zips for 1 component.
We might not even support that in build scripts.

@adityaj1107
Copy link
Contributor

Also regarding the CI, the backend one is failing because common-utils has changes that Notifications doesn't have yet (it's in PR). I believe the frontend CI is failing due to the OpenSearch Dashboard issues (some of the recent commits broke the bootstrapping of our plugin).

PR for common-utils: opensearch-project/common-utils#156

@adityaj1107
Copy link
Contributor

Please make sure you have a second PR to remove default scripts from build repo.
Thanks.

opensearch-project/opensearch-build#1938

@qreshi qreshi merged commit 848add9 into opensearch-project:main Apr 7, 2022
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.

3 participants