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

Adjusted nginx configuration based on the example from Nextcloud for Version 26 #258

Merged
merged 7 commits into from
May 2, 2023
Merged

Adjusted nginx configuration based on the example from Nextcloud for Version 26 #258

merged 7 commits into from
May 2, 2023

Conversation

seikexyz
Copy link
Contributor

This updated nginx configuration should now be compatible with Nextcloud 26.

Closes #246

@staticdev
Copy link
Collaborator

staticdev commented Apr 22, 2023 •
edited
Loading

Thanks for the contribution @seikexyz! Could you please sign the commits with DCO as you can see needed for this repo?

@seikexyz
Copy link
Contributor Author

Sorry, this is my first PR here. What would the best way to do so? I tried to push a new commit (23469ab but it did not work. Or would it be best to close the request and open a new one, properly signed this time?

@aalaesar
Copy link
Member

Sorry, this is my first PR here. What would the best way to do so? I tried to push a new commit (23469ab but it did not work. Or would it be best to close the request and open a new one, properly signed this time?

Hello there! Thank you for your contribution.
To fix the DCO, click on "details" in the check reports, on the DCO line, then follow the instructions.
Basically, you'll have to rebase the branch with proper git option to signoff the commits.

Regards.

@wiktor2200 wiktor2200 added the enhancement New feature or request label Apr 25, 2023
@wiktor2200
Copy link
Collaborator

Hi @seikexyz! Thanks for your contribution!
You can easily fix DCO sign-off for whole branch just by running commands in section Rebase the branch when clicked on failed DCO job status Details: https://github.com/nextcloud/ansible-collection-nextcloud-admin/pull/258/checks?check_run_id=12844472948

And regarding MR, I've got some comments. In general settings was based on this document: https://docs.nextcloud.com/server/latest/admin_manual/installation/nginx.html#nextcloud-in-the-webroot-of-nginx
and in the past I was just doing diff command to fast-forward new upcoming changes to Nginx settings.
I can see that they changed some content of this file (all your changes) but also order of some parameters (cosmetic changes), but really helpful when just doing simple diff.
So I've got a question, do you want to solve it on your own or do you want me to help and assist? :)

seikexyz and others added 2 commits April 26, 2023 21:07
@seikexyz
Adapted from official NC doc
91312fb 
Changed X-Robot-Tag and added immutable cache control options.

Signed-off-by: seikexyz <[email protected]>
@seikexyz
Missing signed-off-statement
3ee8429 
Signed-off-by: seikexyz <[email protected]>

Signed-off-by: Stefan <[email protected]>
Signed-off-by: seikexyz <[email protected]>
@seikexyz seikexyz force-pushed the (fix)-NGINX-Config branch from 23469ab to 3ee8429 Compare April 26, 2023 19:11
@seikexyz
Copy link
Contributor Author

Hi @wiktor2200 & @aalaesar thank you for the hints on how to rebase the branch. It worked now!

@wiktor2200 I can solve it :)

aalaesar
aalaesar previously approved these changes Apr 29, 2023
View reviewed changes
Copy link
Member

@aalaesar aalaesar left a comment

Choose a reason for hiding this comment

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

LGTM

@seikexyz
Copy link
Contributor Author

Except for some SSL configurations, which do not appear in the documentation, the rest of the template should now be more or less identical.

wiktor2200
wiktor2200 requested changes Apr 30, 2023
View reviewed changes
Copy link
Collaborator

@wiktor2200 wiktor2200 left a comment

Choose a reason for hiding this comment

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

Hi @seikexyz! It looks good, but I found some small typos. Please take a look on my comments.

roles/install_nextcloud/templates/nginx_nc.j2 Outdated Show resolved Hide resolved
roles/install_nextcloud/templates/nginx_nc.j2 Outdated Show resolved Hide resolved
roles/install_nextcloud/templates/nginx_nc.j2 Outdated Show resolved Hide resolved
@wiktor2200
Copy link
Collaborator

wiktor2200 commented Apr 30, 2023 •
edited
Loading

Except for some SSL configurations, which do not appear in the documentation

SSL configs comes from Mozilla SSL best practices guide to get A+ mark in Nextcloud security scan: https://scan.nextcloud.com/

seikexyz and others added 2 commits April 30, 2023 14:32
@seikexyz
Copy link
Contributor Author

@wiktor2200 thanks for the feedback! All typos should hopefully be changed now.

@seikexyz seikexyz requested a review from wiktor2200 April 30, 2023 12:45
wiktor2200
wiktor2200 approved these changes Apr 30, 2023
View reviewed changes
Copy link
Collaborator

@wiktor2200 wiktor2200 left a comment

Choose a reason for hiding this comment

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

All good now. Thanks again for your contribution! :)

@wiktor2200 wiktor2200 merged commit f83f2ac into nextcloud:main May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wiktor2200 wiktor2200 wiktor2200 approved these changes

@aalaesar aalaesar aalaesar left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update web servers X-Robots-Tag header
4 participants
@seikexyz @staticdev @aalaesar @wiktor2200