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 20-app.ini in restore #44

Merged
merged 2 commits into from Dec 17, 2017

Conversation

Projects
None yet
3 participants
@maniackcrudelis
Copy link
Contributor

commented Dec 11, 2017

Problem

  • Backup and restore forget 20-$app.ini, if you try an upgrade after that, it would fail because this file is missing.

Solution

  • Add the lines for this file in backup and restore
  • And, by the way, update these scripts and fix #38

PR Status

Work finished. Package_check, basic tests and upgrade from last version OK.
Could be reviewed and tested.

Validation


Minor decision

  • Upgrade previous version : JimboJoe
  • Code review : JimboJoe
  • Approval (LGTM) : JimboJoe
  • Approval (LGTM) :
  • CI succeeded : Build Status
    When the PR is mark as ready to merge, you have to wait for 3 days before really merge it.
@JimboJoe
Copy link
Contributor

left a comment

LGTM, code review (modulo one minor comment) and test OK 👍

@@ -31,8 +31,10 @@ db_name=$(ynh_app_setting_get $app db_name)
# CHECK IF THE APP CAN BE RESTORED
#=================================================

CHECK_DOMAINPATH # Check domain and path availability
CHECK_FINALPATH # Check if destination directory is not already in use
yunohost app checkurl "${domain}${path_url}" -a "$app" \

This comment has been minimized.

Copy link
@JimboJoe

JimboJoe Dec 12, 2017

Contributor

Here we could use ynh_webpath_available.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2017

Here, the CI fail because of the script change_url, see here YunoHost/yunohost#394.
To fix that, I use a temporary copy of this helper.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2017

@JimboJoe , if you agree, let's merge this PR in 3 days.
The failling of the CI is not related to this PR. But it would be interested to make another PR to fix that.

@JimboJoe

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

Agreed!

@lapineige

This comment has been minimized.

Copy link
Member

commented Dec 14, 2017

I agree too, after testing, it does solve the problem and I don't see any drawbacks.

@maniackcrudelis maniackcrudelis merged commit b2968c6 into testing Dec 17, 2017

@maniackcrudelis maniackcrudelis deleted the fix_restore branch Dec 17, 2017

@lapineige lapineige referenced this pull request Mar 7, 2019

Closed

Use latest backup helpers #38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.