-
Notifications
You must be signed in to change notification settings - Fork 271
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
Support multiple artifacts per bundled component. #1956
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1956 +/- ##
============================================
- Coverage 94.46% 94.40% -0.06%
Complexity 22 22
============================================
Files 179 180 +1
Lines 3649 3702 +53
Branches 29 29
============================================
+ Hits 3447 3495 +48
- Misses 196 201 +5
Partials 6 6
Continue to review full report at Codecov.
|
The scripts have actually already been moved to Notifications, there are just two minor PRs open to make some changes to them (opensearch-project/notifications#400 and opensearch-project/notifications#401). So we can merge those in and remove the scripts in this PR if you like. Also, the installation order does matter. I also brought this up in the tracking issue and you mentioned it here as well, it could be that the build manifest being outputted here happened to list the plugins in order:
But is this order deterministic? |
Also @dblock I think @peterzhuamazon was looking into the option of listing separate entries in the manifest and supplying a different |
Ok, I see, deleting this from this PR.
I think it's repeatable in the order files are written, but I don't think we can rely on it. |
I went down that rabbit hole first and it seemed like it's a smaller amount of changes. Then I thought that it would be great to support multiple artifacts produced by a single build anyway, so I did this change. @peterzhuamazon Did you make it work with |
I was not sure if you are working on this issue. |
Signed-off-by: dblock <dblock@dblock.org>
56c0791
to
029f416
Compare
What do we like better @peterzhuamazon @qreshi? The pros of this PR is that we can publish multiple ZIPs per component with it, so feels like a more generic fix. The cons is that it's a bigger change, the order is still an issue, manifest checks may need work, and may need to be made more predictable, and the bundle manifest schema has changed. |
As this PR requires a lot more changes and probably will break a thing or two after the push, combined with the fact that notifications situation is a special case, I vote for #1957 as that is the minimum change for maximum outcome. |
The #1957 not only allow use to define the order, but also resolved the issue without adding existing code to further complicated the code base for a one time use case. |
Closing in favor of #1957 |
Signed-off-by: dblock dblock@dblock.org
Description
Added support for multiple artifacts per distribution component.
With this change, the notifications plugin collects 2 ZIPs instead of 1, publishes and installs both.
Build Manifest
Bundle Manifest
It doesn't seem like the order of installation between
notifications
andnotifications-core
matters, but it could be that they are installed in the right order "accidentally". If there's an issue with this I can fix it in a future PR.Issues Resolved
Closes #1950.
Will need to move the custom
build.sh
into notifications repo asscript/build.sh
after merging.Check List
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.