-
Notifications
You must be signed in to change notification settings - Fork 273
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 support for passing snapshot build options. #189
Conversation
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.
lgtm, few minor questions.
I wouldn't worry too much about certain repos not tagging snapshot builds unless they want their snapshot artifacts to end up in maven.
[ -z "$OUTPUT" ] && OUTPUT=artifacts | ||
|
||
mkdir -p $OUTPUT | ||
echo ./gradlew assemble --no-daemon --refresh-dependencies -DskipTests=true -Dopensearch.version=$VERSION -Dbuild.snapshot=$SNAPSHOT |
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 echo intentional?
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, cause I can never tell what we're running where.
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.
Consider using set -x
instead: it's very easy to update the command but not the echo statement. Not a blocker; just a suggestion for the future.
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.
Great point. I'll remove the echo for now and address whether we want to set -x -e
everywhere in a separate PR.
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 change looks fine, just a couple of suggested tweaks for the future.
There is, however, a lot going on in this PR: Changing from sys.argv to ArgumentParser, plumbing the snapshot flag through, changing the 'artifacts' directory name from a hard-coded string to a variable, changing the way manifests read from files, updating the component build scripts to consume the snapshot flag, some inconsequential style changes (such as putting then
on the same line as the if
in a shell script). Most of these changes are unrelated.
In future, consider splitting unrelated changes into their own PRs in order to reduce cognitive load on the reviewers, and to ensure that each commit is a small, self-contained change that can be easily reverted if it turns out there is a problem.
@@ -53,9 +53,9 @@ def get_arch(): | |||
component_scripts_path, | |||
default_build_path, | |||
build_recorder) | |||
builder.build(manifest.build.version, arch) | |||
builder.build(manifest.build.version, arch, args.snapshot) |
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.
If we start adding any more arguments here we should consider capturing them in a class, let's say BuildSpec
.
builder.export_artifacts() | ||
|
||
build_recorder.write_manifest(output_dir) | ||
|
||
print('Done') | ||
print('Done.') |
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.
Please remove the period
😛
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.
He didn't!
@@ -24,8 +24,9 @@ def __init__(self, component_name, git_repo, component_scripts_path, default_bui | |||
self.component_scripts_path = component_scripts_path | |||
self.default_build_script = default_build_script | |||
self.build_recorder = build_recorder | |||
self.output_path = 'artifacts' |
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.
Make this a constant declared at the class level:
class Builder:
OUTPUT_PATH = 'artifacts'
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 think this actually should become input as a fully qualified path, so you can redirect things and not always create a hard-coded artifacts dir from where you run things. I didn't want to pollute this PR with that, though.
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: dblock <dblock@amazon.com>
…#189) * Refactor argument parsing to use argparse. * Added support for snapshot builds. * Fix: typo. * Record -SNAPSHOT version. * Removed echo. Signed-off-by: dblock <dblock@amazon.com>
Description
Added support for passing
--snapshot
to the bundle build, which cascades-s
which cascades into-dbuild.snapshot=true
into gradle/maven builds. Opened some issues to fix the actual snapshot builds for a handful of plugins, but that doesn't need to hold up this PR.Making the custom override scripts actually parse arguments so that we can extend them in the future with additional parameters and not rely on the fact that arg1 is OpenSearch version and arg2 is the architecture.
Issues Resolved
Part of #179
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.