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

Remove features and feature_list usage in Notifications #382

Merged
merged 4 commits into from
Mar 18, 2022

Conversation

qreshi
Copy link
Contributor

@qreshi qreshi commented Mar 16, 2022

Description

  • Updates Kotlin to 1.6.10 (this was to be consistent with common-utils and caused compilation errors which were also fixed in this PR)
  • Removes feature and feature_list usage in backend and frontend

This PR is making changes to integrate the changes in opensearch-project/common-utils#136

Note: During manual testing on the frontend it became apparent that the frontend is incorrectly forming an Email channel object (the recipient is a list of Strings instead of list of EmailRecipient objects i.e { "recipient": "[email protected]" } like the backend expects). We may favor the frontend format here since wrapping it in an object seems unnecessary. In either case, this will be addressed in a follow-up PR.

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.

@qreshi qreshi requested a review from a team March 16, 2022 02:17
@qreshi
Copy link
Contributor Author

qreshi commented Mar 16, 2022

Also since the GitHub Actions for building the backend is still failing (since common-utils 2.0 snapshot is not available yet) I ran the build locally which passed:

notifications % ./gradlew build
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 7.3
  OS Info               : Mac OS X 10.16 (x86_64)
  JDK Version           : 14 (OpenJDK)
  JAVA_HOME             : /Library/Java/JavaVirtualMachines/jdk-14.0.1.jdk/Contents/Home
  Random Testing Seed   : 2F8DD9510DED255E
  In FIPS 140 mode      : false
=======================================

BUILD SUCCESSFUL in 5m 42s
55 actionable tasks: 13 executed, 42 up-to-date

@dbbaughe
Copy link
Contributor

dbbaughe commented Mar 16, 2022

Also since the GitHub Actions for building the backend is still failing (since common-utils 2.0 snapshot is not available yet) I ran the build locally which passed:

notifications % ./gradlew build
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 7.3
  OS Info               : Mac OS X 10.16 (x86_64)
  JDK Version           : 14 (OpenJDK)
  JAVA_HOME             : /Library/Java/JavaVirtualMachines/jdk-14.0.1.jdk/Contents/Home
  Random Testing Seed   : 2F8DD9510DED255E
  In FIPS 140 mode      : false
=======================================

BUILD SUCCESSFUL in 5m 42s
55 actionable tasks: 13 executed, 42 up-to-date

What is the hold up for having the 2.0 snapshot published for common utils?

@qreshi
Copy link
Contributor Author

qreshi commented Mar 16, 2022

What is the hold up for having the 2.0 snapshot published for common utils?

I think from the recent plugins that were added to the 2.0 build manifest, first it was Job Scheduler (which was fixed), then it seems that Alerting was added prematurely and was failing it because it wasn't upgraded to 2.0. That was the state of things last I checked.

@dbbaughe
Copy link
Contributor

What is the hold up for having the 2.0 snapshot published for common utils?

I think from the recent plugins that were added to the 2.0 build manifest, first it was Job Scheduler (which was fixed), then it seems that Alerting was added prematurely and was failing it because it wasn't upgraded to 2.0. That was the state of things last I checked.

Ok, maybe if Alerting is failing it we can remove Alerting from the 2.0 manifest so Common utils can get successfully published as it's required by a lot of plugins.

Copy link
Contributor

@dbbaughe dbbaughe left a comment

Choose a reason for hiding this comment

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

LGTM, up to you if you want to wait for common utils 2.0 snapshot to get published.

@qreshi
Copy link
Contributor Author

qreshi commented Mar 18, 2022

I see the common-utils 2.0 snapshot is available now. The backend test workflow is failing on some flaky tests that seems to be difficult to reproduce locally. I'll merge this change in for now and look at fixing the flaky tests in a follow-up PR.

@qreshi qreshi merged commit 6a0e36b into opensearch-project:main Mar 18, 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