8000 Create a dedicated upgrade command by stnguyen90 · Pull Request #5860 · appwrite/appwrite · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Create a dedicated upgrade command #5860

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

Closed
wants to merge 1 commit into from
Closed

Conversation

stnguyen90
Copy link
Contributor
@stnguyen90 stnguyen90 commented Jul 21, 2023

What does this PR do?

Before, we used the same command for both installation and upgrades. This lead to problems because developers would try to upgrade in the wrong folder and end up creating a new installation.

This new upgrade command validates the existence of an existing installation before proceeding with the upgrade to ensure no new installation is created when upgrading.

Test Plan

The following screenshot shows:

  1. Running the upgrade command w/o an installation results in an error
  2. After adding an installation (appwrite folder w/ docker-compose.yml file), the install command runs as usual
    • The error at the end occurs because I'm running in an appwrite instance where the docker socket is unavailable. When someone tries to upgrade, they will have the docker socket available and this error won't show.
Screen Shot 2023-07-20 at 7 11 29 PM

Latest error output:

image

Related PRs and Issues

Checklist

  • Have you read the Contributing Guidelines on issues?
  • If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

@stnguyen90 stnguyen90 requested review from gewenyu99 and eldadfux July 21, 2023 18:29
Before, we used the same command for both installation and upgrades.
This lead to problems because developers would try to upgrade in the
wrong folder and end up creating a new installation.

This new upgrade command validates the existence of an existing
installation before proceeding with the upgrade to ensure no new
installation is created when upgrading.
@stnguyen90 stnguyen90 force-pushed the feat-upgrade-command branch from 8bbe26b to ffea39c Compare July 21, 2023 19:38
@stnguyen90 stnguyen90 marked this pull request as ready for review July 21, 2023 21:21
Copy link
Member
@eldadfux eldadfux left a comment

Choose a reason for hiding this comment

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

@stnguyen90 this is not exactly how we use class based tasks, we've created a new abstraction layer for them here: https://github.com/appwrite/cloud/tree/feat-billing/src/Appwrite/Cloud/Platform/Tasks we might be able to reuse this logic as we need to rewrite them anyway.

Please check with @lohanidamodar if we can adapt this approach here.

Copy link
Contributor
@gewenyu99 gewenyu99 left a comment

Choose a reason for hiding this comment

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

LGTM, but I want to add one word to feel useful.

(Jokes aside I think we should be verbose here.

if (empty($data)) {
Console::error('Appwrite installation not found.');
Console::log('The command was not run in the parent folder of your appwrite installation.');
Console::log('Please navigate to the parent of the Appwrite installation and try again.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Console::log('Please navigate to the parent of the Appwrite installation and try again.');
Console::log('Please navigate to the parent directory of the Appwrite installation and try again.');

@stnguyen90
Copy link
Contributor Author

Need to restructure the task.

@stnguyen90 stnguyen90 closed this Jul 24, 2023
@stnguyen90 stnguyen90 deleted the feat-upgrade-command branch September 10, 2023 16:34
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
0