-
Notifications
You must be signed in to change notification settings - Fork 207
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
Create a runnable distribution from assemble task #1774
Conversation
tasks.withType(Zip) { | ||
enabled false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This disables the tasks to build .zip
distributions. I assume those are not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not build zip
archives presently.
if(startParameter.getProjectProperties().containsKey('release')){ | ||
include 'release' | ||
include 'release:archives' | ||
include 'release:archives:linux' | ||
include 'release:docker' | ||
include 'release:maven' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the recent directory change, I think this conditional is not needed anymore. We need release
projects included to assemble a runnable data prepper because the data-prepper
shell script is inside release
folder.
Anything I may be missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree here that we shouldn't require this property anymore.
The only possible downsides I would see are: 1) If the release build requires build parameters the build would fail, but that seems to not be the case; 2) Users might have to download a few extra dependencies to build (e.g. Docker), but that should be quite minor.
I think the benefit of a streamlined command outweighs those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not required now because we want to create a runnable distribution even with ./gradlew assembe
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to update our documentation here:
https://github.com/opensearch-project/data-prepper/blob/main/docs/developer_guide.md#running-the-project
Can be in this PR or a follow-on.
if(startParameter.getProjectProperties().containsKey('release')){ | ||
include 'release' | ||
include 'release:archives' | ||
include 'release:archives:linux' | ||
include 'release:docker' | ||
include 'release:maven' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree here that we shouldn't require this property anymore.
The only possible downsides I would see are: 1) If the release build requires build parameters the build would fail, but that seems to not be the case; 2) Users might have to download a few extra dependencies to build (e.g. Docker), but that should be quite minor.
I think the benefit of a streamlined command outweighs those.
if(startParameter.getProjectProperties().containsKey('release')){ | ||
include 'release' | ||
include 'release:archives' | ||
include 'release:archives:linux' | ||
include 'release:docker' | ||
include 'release:maven' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not required now because we want to create a runnable distribution even with ./gradlew assembe
?
7831834
to
00d785a
Compare
tasks.withType(Zip) { | ||
enabled false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not build zip
archives presently.
Signed-off-by: Hai Yan <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
00d785a
to
79f4360
Compare
Signed-off-by: Hai Yan <[email protected]>
Found some unit test failures after rebasing on latest main. Pushed a commit to fix them. Thanks @asifsmohammed for help with the fix. |
Codecov Report
@@ Coverage Diff @@
## main #1774 +/- ##
============================================
- Coverage 94.11% 93.99% -0.13%
- Complexity 1423 1431 +8
============================================
Files 187 187
Lines 4180 4213 +33
Branches 336 339 +3
============================================
+ Hits 3934 3960 +26
- Misses 167 173 +6
- Partials 79 80 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Hai Yan [email protected]
Description
This PR improves the
assemble
gradle task. It now calls an additional install task provided by thedistribution
plugin to create a runnable distribution. With this change, we can build and run data prepper by:./gradlew assemble
release/archives/linux/build/install/opensearch-data-prepper-{VERSION}-linux-x64
and runbin/data-prepper
Issues Resolved
Resolves #1762
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.