-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add tasks to build separate releases for each storage engine. #376
Conversation
…ks depend on sources instead of utility package task.
@haiqi96 note the task name changes. |
LGTM. Let me merge #363 first and re-validate this PR. |
Thanks for the note, need to think about how this fits into my flow. |
Based on our offline discussion, I've reverted to building a single package and only building clp-json/text releasable tar balls. |
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 with the creations of Tarballs with different default config values.
Taskfile.yml
Outdated
deps: ["package"] | ||
cmds: | ||
- "rm -rf '{{.OUTPUT_DIR}}' '{{.OUTPUT_FILE}}'" | ||
- "cp -r '{{.G_PACKAGE_BUILD_DIR}}' '{{.OUTPUT_DIR}}'" |
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.
Shall we use rsync?
@@ -328,6 +319,43 @@ tasks: | |||
URL_PREFIX: "{{.NODEJS_VERSION_BASE_URL}}" | |||
OUTPUT_DIR: "{{.OUTPUT_DIR}}" | |||
|
|||
package-tar: |
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.
It might be trivial: can we remove the .gitignore
in the package before packaging it? It might be confusing to see the file present in the released tar.
(On the other hand, is there a reason we put an empty .gitignore
in components/package-template?)
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.
Ah, it's obsolete so I removed it.
Co-authored-by: Junhao Liao <[email protected]>
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.
in general looks good to me. Just have two comments but I think they are minor
@@ -76,7 +67,7 @@ tasks: | |||
- "webui" | |||
- "webui-nodejs" | |||
cmds: | |||
- task: "clean-package" | |||
- "rm -rf '{{.OUTPUT_DIR}}'" |
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.
Just thinking out loud, we have talked about having a package where task overwrites and rsync it to another package where we actually run start_clp to avoid removing the database and compressed archive. Any chance we can add this flow to the task file?
Anyway, it doesn't need to be in this 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.
Yeah, I thought about it a bit, but just haven't done any work on it. I'll add a task for it.
@@ -35,28 +35,19 @@ tasks: | |||
vars: | |||
COMPONENT: "job-orchestration" | |||
|
|||
clean-package: |
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.
Are we changing the behavior such that we can't run task:clean-package is not valid target anymore?
It makes sense for me since anyway running task package will just remove the old package anyway. but what if we want to add more fancy checks, such as make sure no existing containers are running?
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.
Yeah, we're removing it for now. If we have more fancy things, I'm happy to add it back.
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. How about:
Add tasks to build separate releases for each storage engine. (#376)
Description
CLP currently has two storage engines:
clp
for managing text logs.clp-s
for managing json logs.This PR adds new Taskfile tasks for building releasable tar balls where the default storage engine is one or the other (the only difference between the package binaries is a config value change). Specifically, this PR adds:
clp-json-pkg-tar
for assembling a versioned tar for theclp-json
packageclp-text-pkg-tar
for assembling a versioned tar for theclp-text
packageAs a result, the
package-tar
tasks has been parameterized and made internal.This PR also removes the
clean-package
task since it was only used in one place and just deleted the package output directory anyway.This PR also adds a utility task for replacing text and ensures that the
validate-checksum
task handles the case where the data to be checksummed doesn't exist.Validation performed
Validated the following sequence of commands were successful:
task clean
task
builds the packagetask
detectedwebui-node-modules
was changed (due to a known issue fixed in Taskfile: Download and set up Meteor.js as part of building the package; Refactor tarball download and extraction into reusable task. #363) and rebuilt the relevant artifactstask
detected no changestask clean
task clp-json-pkg-tar
built the clp-json packagetask clp-text-pkg-tar
built the clp-text packageValidated that
clp-json-package
used theclp-s
storage engine andclp-text-package
used theclp
storage engine.Validated that compression and search were functional for both
clp-json-package
andclp-text-package
.