Fix --port setting #13288
Fix --port setting #13288
Conversation
Unfortunately there was an error in go-gitea#13195 which set the --port option before the settings were read. This PR fixes this by moving applying this option to after the the settings are read However, on looking further into this code I believe that the setPort code was slightly odd. Firstly, it may make sense to run the install page on a different temporary port to the full system and this should be possible with a --install-port option. Secondy, if the --port option is provided we should apply it to both otherwise there will be unusual behaviour on graceful restart Thirdly, the documentation for --port says that the setting is temporary - it should therefore not save its result to the configuration (This however, does mean that authorized_keys and internal links may not be correct. - I think we need to discuss this option further.) Fix go-gitea#13277 Signed-off-by: Andrew Thornton <[email protected]>
GiteaBot
added
the
lgtm/need 2
cmd/web.go
Outdated
|
|
||
| if err := cfg.SaveTo(setting.CustomConf); err != nil { | ||
| return fmt.Errorf("Error saving generated JWT Secret to custom config: %v", err) | ||
| } |
There was a problem hiding this comment.
--port states that it is temporary so it shouldn't write to the ini...
however, this is going to break gitea serv, admin, manager etc. I just don't know what to do here.
In terms of doing a pure bugfix for the linked Issue only this change should be dropped but it's really hard to know what to do here.
Codecov Report
@@ Coverage Diff @@
## master #13288 +/- ##
==========================================
- Coverage 42.16% 42.11% -0.06%
==========================================
Files 685 689 +4
Lines 75571 75851 +280
==========================================
+ Hits 31867 31947 +80
- Misses 38482 38669 +187
- Partials 5222 5235 +13
Continue to review full report at Codecov.
|
| @@ -40,6 +40,7 @@ Starts the server: | |||
|
|
|||
| - Options: | |||
| - `--port number`, `-p number`: Port number. Optional. (default: 3000). Overrides configuration file. | |||
| - `--install-port number`: Port number to run the install page on. Optional. (default: 3000). Overrides configuration file. | |||
There was a problem hiding this comment.
I think this change could be another PR.
There was a problem hiding this comment.
It's part of the fix really.
If you make --port apply both before the install page and after the install page then the PORT option in the install page is irrelevant and it is overwritten.
If you make --port only apply once then if there is a graceful restart after install the port that gitea runs on will change.
If you make --port only apply to the install page then that may change behaviour for other people.
In this case the only solution is provide a separate option for the install page then you can have a consistent solution.
GiteaBot
added
lgtm/need 1
GiteaBot
added
lgtm/done
|
🚀 |
Unfortunately there was an error in #13195 which set the --port
option before the settings were read. This PR fixes this by
moving applying this option to after the the settings are read
However, on looking further into this code I believe that the setPort
code was slightly odd.
Firstly, it may make sense to run the install page on a different
temporary port to the full system and this should be possible with
a --install-port option.
Secondy, if the --port option is provided we should apply it to both
otherwise there will be unusual behaviour on graceful restart
Thirdly, the documentation for --port says that the setting is
temporary - it should therefore not save its result to the configuration
(This however, does mean that authorized_keys and internal links may
not be correct. - I think we need to discuss this option further.)
Fix #13277
Signed-off-by: Andrew Thornton [email protected]