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 port for ynh_add_nginx_config #134

Closed
wants to merge 1 commit into from
Closed

Conversation

Krakinou
Copy link

@Krakinou Krakinou commented Oct 9, 2020

Problem

  • When __ PORT__ is used in nginx.conf, ynh_add_nginx_config will not changed it if $port is not initialized with the port number. This will cause nginx to fail at reload.

Solution

  • Initialize $port from the app settings. If port is not set in app settings, nothing will happen

PR Status

  • Code finished.
  • Tested with Package_check.
  • Fix or enhancement tested.
  • Upgrade from last version tested.
  • Can be reviewed and tested.

@yalh76
Copy link
Contributor

yalh76 commented Oct 14, 2020

There is no __PORT__ in the nginx.conf provided in example_ynh, so that could be strange for a new developper...
In addition, in that case $port initialisation is missing in the upgrade script.

For me, example_ynh should be as generical as possible without implementing all possibilities but providing a good package start and main guidelines

@ericgaspar
Copy link
Member

@Krakinou as a point. It is confusing not to mention $port variable... this variable should at least appear as a commented option. (like the database variables L.32-35) I encountered several time errors in the change_url script before realizing that the port variable was missing ...

@alexAubin
Copy link
Member

alexAubin commented Jan 6, 2021

(As a followup, related to YunoHost/yunohost#1118 , maybe ynh_add_config could report an error if it found some __FOOBAR__ to be replaced but no corresponding variable to replace it with)

@yalh76
Copy link
Contributor

yalh76 commented Apr 29, 2021

Now ynh_add_config report an error when an argument is missing :)

@yalh76 yalh76 closed this Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants