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.

Sign up for GitHub

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

Jump to bottom

Testing : update to 0.12.0, nginx fix, other fixes #58

Merged
merged 18 commits into from
Nov 30, 2020
Merged

Testing : update to 0.12.0, nginx fix, other fixes #58

merged 18 commits into from
Nov 30, 2020

Conversation

lapineige
Copy link
Member

@lapineige lapineige commented Nov 3, 2020 •
edited by ericgaspar
Loading

#57 : Updated to V0.12.0 and Nginx fix
#59

Build Status

tituspijean and others added 4 commits April 17, 2020 14:44
Set file permissions after creating the log (#53)
1d1e3d8 
Should fix #49
@anmol26s @lapineige @ericgaspar
Updated to V0.12.0 and Nginx fix (#57)
fa25f18 
* Set file permissions after creating the log (#53) (#54)

Should fix #49

Authored-by: tituspijean <tituspijean@outlook.com>

* Updated to version v0.12.0

* Fix images

* Fixed path in nginx

Co-authored-by: lapineige <lapineige@users.noreply.github.com>
Co-authored-by: anmol <anmol@jswan2.com>
Co-authored-by: Éric Gaspar <46165813+ericgaspar@users.noreply.github.com>
@ericgaspar
Update README.md
24cb67f
@lapineige
Merge branch 'master' into testing
384df50
ericgaspar
ericgaspar requested changes Nov 10, 2020
View reviewed changes
Copy link
Member

@ericgaspar ericgaspar left a comment

Choose a reason for hiding this comment

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

Quick review...
You can also remove the comments on install script...

issue_template.md Outdated Show resolved Hide resolved
scripts/_common.sh Outdated Show resolved Hide resolved
lapineige and others added 4 commits November 10, 2020 20:55
@lapineige @ericgaspar
Update issue_template.md
99f39cf 
Co-authored-by: Éric Gaspar <46165813+ericgaspar@users.noreply.github.com>
@lapineige @ericgaspar
Update openssl dependency
dd3251e 
Co-authored-by: Éric Gaspar <46165813+ericgaspar@users.noreply.github.com>
@ericgaspar
Update _common.sh
0999a92 
- Remove unused script that generates an error
- Remove openssl that is part of PHP as default
@ericgaspar
Cleaning up
03225c7
@anmol26s
Copy link
Contributor

Why is CI failing?

alexAubin
alexAubin approved these changes Nov 24, 2020
View reviewed changes
Copy link
Member

@alexAubin alexAubin left a comment

Choose a reason for hiding this comment

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

Super quick review, LGTM

@lapineige
Copy link
Member Author

@anmol26s I don't get it https://ci-apps-dev.yunohost.org/jenkins/job/shaarli_ynh%20PR58%20(ericgaspar)/lastBuild/console

@alexAubin
Copy link
Member

@lapineige : uh yeah for some reason it looks like the CI fails to reach the app after the (successful) installation:

Try to access by url...
curl: (47) Maximum (50) redirects followed

Dunno if that's due to the CI somehow or if an human is able to reproduce the issue...

@ericgaspar
Copy link
Member

ericgaspar commented Nov 24, 2020 •
edited
Loading

You can implement the cURL post-install explained by Kayou on this issue for Mantis:
YunoHost-Apps/mantis_ynh#6
and the code:
https://github.com/YunoHost-Apps/mantis_ynh/blob/e7a1f471d813a829558fe426aa21beb124f76727/scripts/install#L121-L137
This will skip the post installation panel

@lapineige
Copy link
Member Author

I don't understand all of that, does it concerns only CI or this app too ?

@ericgaspar
Copy link
Member

I think this is a problem with the NGINX config ...

@ericgaspar
Patch2 (#61)
837c394 
- Fix NGINX 302 redirect
- Fix missing upgrade_type in upgrade script
- Add ynh_smart_mktemp helper
@ericgaspar
Copy link
Member

ericgaspar commented Nov 24, 2020 •
edited
Loading

@lapineige I pushed Patch2 which should fix some errors
Edit: the 302 is gone but the NGINX needs more work. It leeds to a white page. :/

@tituspijean
Copy link
Member

So, I have investigated the issue with the Too many redirects. It happens due to Shaarli making the browser go back and forth between /install and /install/session-test. (I suspect it is a feature, not a bug, but I fail to see why they check for a session that way).

If we reinstate conf/config.json.php, the postinstall will be ignored and redirects loop will never be triggered. We need to update this file though, and put the placeholders seds (or --other_vars) back in install and upgrade.

@lapineige
Copy link
Member Author

lapineige commented Nov 25, 2020 •
edited
Loading

I'll trust you on this one, this is beyond my knowledge in yunohost packaging and nginx stuff. Just tell me when it's fixed :)

@tituspijean
Copy link
Member

tituspijean commented Nov 26, 2020 •
edited
Loading

Build Status as of 9a949c1.

(cannot re-request a review, so let me ping @ericgaspar)

alexAubin
alexAubin approved these changes Nov 27, 2020
View reviewed changes
Copy link
Member

@alexAubin alexAubin left a comment

Choose a reason for hiding this comment

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

Super quick review, LGTM

@lapineige lapineige changed the title Testing Testing : update to 0.12.0, nginx fix, other fixes Nov 30, 2020
@lapineige lapineige merged commit 33d286f into master Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexAubin alexAubin alexAubin approved these changes

@ericgaspar ericgaspar ericgaspar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lapineige @anmol26s @alexAubin @ericgaspar @tituspijean