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

panel: Reorder, fixes, certbot with --nginx #100

Merged
merged 58 commits into from
Jan 14, 2021
Merged

panel: Reorder, fixes, certbot with --nginx #100

merged 58 commits into from
Jan 14, 2021

Conversation

Linux123123
Copy link
Member

@Linux123123 Linux123123 commented Oct 12, 2020

Big reorder, added more dividers, made code simpler to read, changed up the order a bit, remade perform_install, remade OS specific installs, removed redundant apt update, removed unused variables, added --nginx instead of --standalone, removed a lot of redundant checks for OS version (if it checks if OS is the correct and that it is supported against a hard coded list, we don't need to recheck that every time we run something).

No SSL ASSUME SSL SSL
Ubuntu 20.04
Ubuntu 18.04
Debian buster
Debian stretch
CentOS 7 *
CentOS 8 *

Fixes #88

@Linux123123
Copy link
Member Author

@vilhelmprytz A thing to say is that when using --nginx when it fails it doesn't edit any files, so if it fails it just makes the site with no ssl.
Also in this PR you can see that there are no nginx_ssl. This is because option --nginx automatically adds all the needed thing for ssl and converts a no ssl config to ssl config.
So basically there is no way to assume_ssl after using certbot with --nginx, because the files aren't edited if certificates fail. I don't see this as a flaw.

@vilhelmprytz
Copy link
Member

@vilhelmprytz A thing to say is that when using --nginx when it fails it doesn't edit any files, so if it fails it just makes the site with no ssl.
Also in this PR you can see that there are no nginx_ssl. This is because option --nginx automatically adds all the needed thing for ssl and converts a no ssl config to ssl config.
So basically there is no way to assume_ssl after using certbot with --nginx, because the files aren't edited if certificates fail. I don't see this as a flaw.

Yes, I see your point. I remember I mentioned I was going to implement certbot --nginx myself separately? This PR is way too big for me to do in one go, you've piled up too much work in one PR. Is it too late to ask if you can split it up in smaller separate PRs?

@Linux123123
Copy link
Member Author

@vilhelmprytz A thing to say is that when using --nginx when it fails it doesn't edit any files, so if it fails it just makes the site with no ssl.
Also in this PR you can see that there are no nginx_ssl. This is because option --nginx automatically adds all the needed thing for ssl and converts a no ssl config to ssl config.
So basically there is no way to assume_ssl after using certbot with --nginx, because the files aren't edited if certificates fail. I don't see this as a flaw.

Yes, I see your point. I remember I mentioned I was going to implement certbot --nginx myself separately? This PR is way too big for me to do in one go, you've piled up too much work in one PR. Is it too late to ask if you can split it up in smaller separate PRs?

A bit late I would say. I have a solution. We can do the review threw discord and quickly go threw all of the stuff that I have done. It would be really hard for me to split this PR.

install-panel.sh Outdated Show resolved Hide resolved
install-panel.sh Outdated Show resolved Hide resolved
install-panel.sh Outdated Show resolved Hide resolved
install-panel.sh Outdated Show resolved Hide resolved
install-panel.sh Outdated Show resolved Hide resolved
install-panel.sh Outdated Show resolved Hide resolved
install-panel.sh Show resolved Hide resolved
install-panel.sh Show resolved Hide resolved
install-panel.sh Outdated Show resolved Hide resolved
install-panel.sh Outdated Show resolved Hide resolved
Linux123123 and others added 2 commits January 2, 2021 11:51
Co-authored-by: Vilhelm Prytz <vilhelm@prytznet.se>
Co-authored-by: Vilhelm Prytz <vilhelm@prytznet.se>
@vilhelmprytz
Copy link
Member

vilhelmprytz commented Jan 2, 2021

Also sorry for taking so long before properly reviewing your PR. When you move a lot of the functions around in conjunction with all the changes, it makes it harder for me to review in one go. 🙂

I think we're closing in on a merge now. I've got to test the script using Let's Encrypt before merge (after all review change requests are resolved).

@Linux123123
Copy link
Member Author

Linux123123 commented Jan 4, 2021

Ok all OS are tested now.

When executed, the exit status of a function is the exit status of the last command executed in the body. Since the expression would return exit 1, the function ask_assume_ssl would cause the script to exit if the user selected not to ASSUME_SSL (the expression is the last command of the function).
vagrant centos images do not have /usr/local/bin in their path so we make sure it is always present
Copy link
Member

@vilhelmprytz vilhelmprytz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@vilhelmprytz vilhelmprytz merged commit 0961a9b into pterodactyl-installer:master Jan 14, 2021
vilhelmprytz added a commit that referenced this pull request Jan 14, 2021
after #100 merge
@Linux123123 Linux123123 deleted the master-panel-PR branch April 6, 2021 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Let's Encrypt --nginx over standalone webserver
2 participants