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

Replace "unix" by "http+unix" for PROTOCOL #17771

Merged
merged 8 commits into from Dec 6, 2021

Conversation

mscherer
Copy link
Contributor

Fixes #9318

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 22, 2021
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Edit I misread the pr I see that "unix" is still supported.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 22, 2021
@zeripath
Copy link
Contributor

Is this really worth doing?

I'd prefer if we could get host and port merged in to an ADDRESS value and then we'd be able to detect if we need to use a unix or a tcp port by the ADDRESS. Then protocol can drop all of the unix variants.

@mscherer
Copy link
Contributor Author

I may be missing something, but a ADDRESS value wouldn't be sufficient, as you still need to specify if that's FCGI or HTTP (or a future protocol), no ?

@zeripath
Copy link
Contributor

Yes you would still need the protocol field but the weird +unix variants would be unnecessary

@mscherer
Copy link
Contributor Author

Yeah, I agree, this would be better. having each potential protocol being double is not elegant and would make doc more complex quite fast if a new protocol is added (eg, something to replace FastCGI).

@silverwind
Copy link
Member

Keep note thathttps+unix is certainly also a possibility.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 24, 2021
@wxiaoguang wxiaoguang added the pr/wip This PR is not ready for review label Nov 24, 2021
@wxiaoguang
Copy link
Contributor

This change is fine to me.

zeripath's suggestion about "get host and port merged in to an ADDRESS" is pretty good.

So should we wait for the new work about unified ADDRESS, or get this PR merged first?

(I marked this PR as wip temporarily)

@mscherer
Copy link
Contributor Author

mscherer commented Dec 1, 2021

I feel that the ADDRESS change would requires a longer design discussion.

@mscherer
Copy link
Contributor Author

mscherer commented Dec 1, 2021

I rebased, but I think it requires a quick re-review

@wxiaoguang
Copy link
Contributor

@zeripath @techknowlogick how do you think?

@wxiaoguang
Copy link
Contributor

Bump. If still no objection in 2 days, maybe we can get this PR merged.

@zeripath
Copy link
Contributor

zeripath commented Dec 4, 2021

yeah I think we should merge as is and address the address issue at another point.

@codecov-commenter

This comment has been minimized.

@lunny
Copy link
Member

lunny commented Dec 4, 2021

Bump. If still no objection in 2 days, maybe we can get this PR merged.

So do you want to remove the wip label?

@wxiaoguang wxiaoguang added type/refactoring Existing code has been cleaned up. There should be no new functionality. and removed pr/wip This PR is not ready for review labels Dec 4, 2021
@techknowlogick techknowlogick merged commit f49d160 into go-gitea:main Dec 6, 2021
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename existing unix protocol to http+unix
8 participants