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

Fix regression for the case of COSMOVISOR_ENABLED without START_CMD #572

Conversation

orenl-lava
Copy link
Contributor

This fixes a glitch in commit 3aacc34.

When starting without args and without START_CMD, the command for regular (as-is) and snapshot are "$PREFIX_CMD $PROJECT_BIN start", whereas for cosmovisor it should be "$PREFIX_CMD start".

Fix using a new DEFAULT_CMD that is set together with PREFIX_CMD based on the selected mode.

This fixes a glitch in commit 3aacc34.

When starting without args and without START_CMD, the command for regular
(as-is) and snapshot are "$PREFIX_CMD $PROJECT_BIN start", whereas for
cosmovisor it should be "$PREFIX_CMD start".

Fix using a new DEFAULT_CMD that is set together with PREFIX_CMD based on
the selected mode.
@PikachuEXE
Copy link
Contributor

I am running one node with both COSMOVISOR_ENABLED and SNAPSHOT_PATH present
Does this new script only run cosmovisor?

@tombeynon
Copy link
Collaborator

I've taken a stab at this in #576 - it retains the previous support for COSMOVISOR_ENABLED AND SNAPSHOT_PATH, and gives the user full control by overriding the docker command if required.

This seems the most sensible route to me, but could you both have a look and make sure it covers all your use cases @orenl-lava @PikachuEXE ?

@orenl-lava
Copy link
Contributor Author

I am running one node with both COSMOVISOR_ENABLED and SNAPSHOT_PATH present
Does this new script only run cosmovisor?

Sorry, I did not consider this possibility.

IIUC, previously if you set both, then the it would execute snapshot.sh cosmovisor run start.
Before fixing this, let's agree on the desired semantics:

Snap = snapshot
Cosm = cosmovisor

BIN = $PROJECT_BIN

           |                                command to execute
  given    |   ----------   ----------------------   -------------------------   -------------------------------------
  args     |     normal             snap                     cosm                           snap+cosm
---------- |   ----------   ----------------------   -------------------------   -------------------------------------
args($@)   |   "$@"         snapshot.sh "$@"         cosmovisor run "$@"         snapshot.sh cosmovisor run "$@"
$START_CMD |   $START_CMD   snapshot.sh $START_CMD   cosmovisor run $START_CMD   snapshot.sh cosmovisor run $START_CMD
no args    |   $BIN start   snapshot.sh $BIN start   cosmovisor run start        snapshot.sh cosmovisor run start

So this supports 4 preset modes of operation (normal, snapshot, cosmovisor, and snapshot+cosmovisor), and 3 types of arguments (no args, command line args, and env-var START_CMD). Anything beyond can be controlled via args or START_CMD.

@PikachuEXE does this cover your use cases? If so, I'll push a fix so we can close this.

@PikachuEXE
Copy link
Contributor

I have tested #576 & responded there so you can check that one first before making changes

@tombeynon
Copy link
Collaborator

I'm going to go ahead with #576 now, partly because we've broken a few things with the previous release so need to get a fix out quickly, and partly because I think it's important the docker command can override any other behaviour. Closing this PR but please re-open against master if anything else needs changing

@tombeynon tombeynon closed this Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants