Skip to content
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

chore(dist): add deploy-release.sh to download & start services #330

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

javeme
Copy link
Contributor

@javeme javeme commented Feb 25, 2024

No description provided.

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Feb 25, 2024
@javeme javeme requested review from imbajin and simon824 February 25, 2024 12:06
@javeme
Copy link
Contributor Author

javeme commented Feb 25, 2024

related to: apache/incubator-hugegraph#2110

Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: We could also let GPT/Dosu to generate/enhance the script


echo "download hugegraph tars from $DOWNLOAD_URL_PREFIX..."
if [[ ! -f "${SERVER_TAR}" ]]; then
wget "${DOWNLOAD_URL_PREFIX}/${RELEASE_VERSION}/${SERVER_TAR}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note wget is not bind in mac or some linux env, better use curl first(instead)

could refer here:
https://github.com/apache/incubator-hugegraph/blob/d0f63c8ac3c0b835560e37404b7b9e271ac0f678/hugegraph-server/hugegraph-dist/src/assembly/static/bin/util.sh#L278

function download() {
    local path=$1
    local download_url=$2
    if command_available "curl"; then
        if [ ! -d "$path" ]; then
            mkdir -p "$path" || {
                echo "Failed to create directory: $path"
                exit 1
            }
        fi
        curl -L "${download_url}" -o "${path}/$(basename "${download_url}")"
    elif command_available "wget"; then
        wget --help | grep -q '\--show-progress' && progress_opt="-q --show-progress" || progress_opt=""
        wget "${download_url}" -P "${path}" $progress_opt
    else
        echo "Required curl or wget but they are unavailable"
        exit 1
    fi
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok let's introduce util.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but there is a problem, we need to download 2 files after util.sh introduction, which is not convenient.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but there is a problem, we need to download 2 files after util.sh introduction, which is not convenient.

Maybe just copy the function directly? (no need to add the whole script file)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it also needs to copy download and command_available, copied code is difficult to maintain, so we prefer to keep it simple

dist/deploy-release.sh Show resolved Hide resolved
Comment on lines 41 to 42
cd $(echo "./*hugegraph-incubating*${RELEASE_VERSION}")
bin/stop-hugegraph.sh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. * in "str" may meet same questions
  2. here, why we need stop server & hubble firstly? (if the old process is running, seems it couldn't kill them in different path?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually the echo would remove the quotes, we can also update to ./*hugegraph-incubating*${RELEASE_VERSION}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stop-hugegraph is just for stop the service also deployed by dist/deploy-release.sh

Copy link
Member

@imbajin imbajin Mar 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stop-hugegraph is just for stop the service also deployed by dist/deploy-release.sh

if someone already download & start the server/hubble processes with the deploy script,

why they need restart them with deploy-*.sh rather than restart the server/hubble directly(with their own script)?

In another case, if a user downloaded & ran the old version 1.0.0 before & now they want to upgrade 1.2.0, the download/running path shall be different & the old processes couldn't be killed in the new path?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps some parameters can be added to the script to indicate whether to stop the service before starting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed stop call

@imbajin imbajin changed the title add dist/deploy-release.sh chore(dist): add deploy-release.sh to download & start services Mar 10, 2024
@imbajin imbajin requested review from liuxiaocs7 and VGalaxies March 10, 2024 09:38

echo "download hugegraph tars from $DOWNLOAD_URL_PREFIX..."
if [[ ! -f "${SERVER_TAR}" ]]; then
wget "${DOWNLOAD_URL_PREFIX}/${RELEASE_VERSION}/${SERVER_TAR}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can also download the corresponding SHA512 file for integrity checking and provide a basis for skipping the download of existing tar packages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal here is just to provide an automated deployment script for developers, so I hope to keep it simple.

Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, keep it simple in V1

We could enhance it later

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 18, 2024
@imbajin imbajin merged commit cebb070 into master Mar 18, 2024
1 check passed
@imbajin imbajin deleted the deploy-release.sh branch March 18, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants