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

Testing #42

Merged
merged 24 commits into from Feb 12, 2018

Conversation

Projects
None yet
4 participants
@JimboJoe
Copy link
Contributor

commented Nov 25, 2017

Problem

Add a change_url script for yunohost app change-url feature.

Solution

  • Change the way the nginx.conf is adapted at install/upgrade: some lines were removed when installed on "/" path --> they must be kept as comments to allow switching to a "/path" path
  • Add a change_url script: as the nginx.conf file is not simple, the code can't be generic and symetric --> every case (/ --> /path, /path --> /, /path --> /path2) is differentiated
  • Depend now on YunoHost 2.7.2 to allow some cleaning: remove sudo prefixes and integrated helpers

These have been splitted in 3 different commits to ease the review process.

PR Status

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

Validation

  • Complete test :
  • Upgrade previous version :
  • Code review : Maniack C
  • Approval (LGTM) : Maniack C
  • Approval (LGTM) :
  • CI succeeded : Maniack C - Level 7 with change_url.

Since this PR has been rerouted from master to testing. It passes to a minor decision.

  • Upgrade previous version : Maniack C
  • Code review : Maniack C
  • Approval (LGTM) : Maniack C
  • Approval (LGTM) : frju365
  • CI succeeded :
  • 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 added some commits Aug 27, 2017

Keep optional configuration lines in nginx.conf to allow changing URL…
… from/to "/path" locations

Rename comment prefixes fore more readability
Merge pull request #41 from YunoHost-Apps/enh_add_change_url
* Keep optional configuration lines in nginx.conf to allow changing URL from/to "/path" locations
Rename comment prefixes fore more readability
* Add change_url script
* Require YNH 2.7.2, remove helpers and sudo prefixes
* Use ynh_replace_string
* Handle nginx conf file checksums
* Fix indentation

@JimboJoe JimboJoe requested a review from YunoHost-Apps/apps-group Nov 25, 2017

@lapineige lapineige self-requested a review Nov 25, 2017

@lapineige

This comment has been minimized.

Copy link
Member

commented Nov 25, 2017

Upgrade works. Change url from /path1 to /path2 works. Changing from domain1 to domain2 works.
It's ok for me :)

lapineige and others added some commits Dec 11, 2017

Merge pull request #44 from YunoHost-Apps/fix_restore
Fix 20-app.ini in restore
@maniackcrudelis

This comment has been minimized.

@maniackcrudelis maniackcrudelis added work needed and removed pending labels Jan 8, 2018

@JimboJoe

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2018

Most probably linked with YunoHost/yunohost#394...

JimboJoe and others added some commits Jan 24, 2018

Merge pull request #43 from YunoHost-Apps/2.3
Update source to 2.3.2
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2018

Upgrade OK, Code review OK, LGTM.

@lapineige lapineige merged commit 689e47f into master Feb 12, 2018

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

Would be better to just say that this PR can be merged instead of merge it without asking.
As a merge in master for an official app, now it's needed to update the official list and update the forum to notify that this app has a new update.

@lapineige

This comment has been minimized.

Copy link
Member

commented Feb 12, 2018

Would be better to just say that this PR can be merged instead of merge it without asking.

Sorry, I understood that you were OK to merge.

I'm working on the forum post (+ social networks).
Can you remind me where I can update the official list ?

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

Can you remind me where I can update the official list ?

You can find the list here

@frju365

This comment has been minimized.

Copy link
Member

commented Feb 12, 2018

3 days is a very short for you. ;) So LGTM for me (even after merging).

@lapineige

This comment has been minimized.

Copy link
Member

commented Feb 12, 2018

Yeah ^^

By the way I'm using it on a daily base for weeks without any problem.

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.