Added support for passing snapshot build options. #189
Conversation
dblock
changed the title
| [ -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.
Yes, cause I can never tell what we're running where.
There was a problem hiding this comment.
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.
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.
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.
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.
Please remove the period
😛
| @@ -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.
Make this a constant declared at the class level:
class Builder:
OUTPUT_PATH = 'artifacts'
There was a problem hiding this comment.
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
--snapshotto the bundle build, which cascades-swhich cascades into-dbuild.snapshot=trueinto 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.