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

Add the backups-create command #452

Merged
merged 8 commits into from Jul 22, 2019
Merged

Add the backups-create command #452

merged 8 commits into from Jul 22, 2019

Conversation

@EtienneM
Copy link
Member

@EtienneM EtienneM commented Jul 17, 2019

Wait for Scalingo/go-scalingo#116 to be merged merged!
Wait for #449 to be merged merged!
Wait for Scalingo/go-scalingo#115 to be merged merged!

╰─$ scalingo --app my-app --addon ad-78fbfa75-367d-43da-98a9-9d40d3ea0dac backups-create
-----> Successfully ordered to make a backup

Fix #450

@EtienneM EtienneM force-pushed the fix/450/backups-create branch from 03446cc to a288f08 Jul 17, 2019
@EtienneM EtienneM force-pushed the fix/450/backups-create branch from 79ab6c8 to ac0bf49 Jul 17, 2019
@EtienneM EtienneM marked this pull request as ready for review Jul 17, 2019
@EtienneM EtienneM requested a review from johnsudaar Jul 17, 2019
return err
}

io.Status("Successfully ordered to make a backup")
Copy link
Member

@johnsudaar johnsudaar Jul 18, 2019

Choose a reason for hiding this comment

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

It would be nice if we could follow the backup progression.

Loading

Copy link
Member Author

@EtienneM EtienneM Jul 18, 2019

Choose a reason for hiding this comment

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

Well actually it was not really well designed. The backend should have returned 202 and an operation URL in the Location header. Then we could have follow the process... ^^

That being said I can add a backups-show command and update the status message to state:

Successfully scheduled a new backup. Type "scalingo backups-show backup-id" to follow the progress

If it's what you want, I would prefer to do it in a separate PR. OK?

Loading

Copy link
Member

@johnsudaar johnsudaar Jul 18, 2019

Choose a reason for hiding this comment

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

	for !backup.IsFinished() {
		spinner.Lock()
		if backup.Status == types.BackupStatusScheduled {
			spinner.Suffix = " Waiting for the backup to start"
		} else {
			spinner.Suffix = " Waiting for the backup to finish"
		}
		spinner.Unlock()

		time.Sleep(1 * time.Second)

		backup, err = client.BackupGet(dbId, backup.ID.Hex())
		if err != nil {
			return errors.Wrap(err, "Fail to refresh backup state")
		}
	}

This is what i did for the db-cli. I think that this might be enough no ?

Loading

Copy link
Member Author

@EtienneM EtienneM Jul 18, 2019

Choose a reason for hiding this comment

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

Do you mean BackupGet or GetBackup? 😆

Loading

Copy link
Member Author

@EtienneM EtienneM Jul 18, 2019

Choose a reason for hiding this comment

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

But yes, good idea!

Loading

@EtienneM
Copy link
Member Author

@EtienneM EtienneM commented Jul 18, 2019

╰─$ scalingo --app my-app --addon ad-78fbfa75-367d-43da-98a9-9d40d3ea0dac backups-create
⣽ Waiting for the backup to finish 

What a good idea 😻

Loading

@EtienneM EtienneM requested a review from johnsudaar Jul 22, 2019
@jonathanperret
Copy link
Contributor

@jonathanperret jonathanperret commented Jul 29, 2019

@johnsudaar @EtienneM FYI this broke one of our scripts that was using the backup-download command. I looks like here (https://github.com/Scalingo/cli/pull/452/files#diff-6eae5f17654b235c68ab2bb17d69c9f2L32) you tried to keep the old backup-download command functional but with a warning; but it looks like the legacy command descriptor is missing the Flags parameter causing it to reject every invocation that has any flag, and not show the warning:

$ scalingo -a my-app backup-download --addon st-12345-67890 --backup abcdef12345
Incorrect Usage: flag provided but not defined: -addon

NAME:
   scalingo backup-download - Download a backup

USAGE:
   scalingo backup-download [command options] [arguments...]

CATEGORY:
   Addons

DESCRIPTION:
     Download a specific backup:
    $ scalingo --app my-app --addon addon_uuid backups-download --backup my_backup

    # See also 'backups' and 'addons'

OPTIONS:
   --region value  Name of the region to use

Fail to run command: flag provided but not defined: -addon

Loading

@EtienneM
Copy link
Member Author

@EtienneM EtienneM commented Jul 29, 2019

@jonathanperret The issue has been fixed an a new bugfix release (v1.15.1) has been released. Sorry for the inconvenience.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants