-
Notifications
You must be signed in to change notification settings - Fork 43
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
Update document: Nginx & URLs path #427
base: development
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis PR adds a new documentation file that explains the configuration of Nginx and URL paths for test and live server environments. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @lcduong - I've reviewed your changes - here's some feedback:
Overall Comments:
- The PR appears to be incomplete - please add the actual documentation content to the new file before requesting review.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ssl_certificate /etc/../fullchain.pem; | ||
ssl_certificate_key /etc/../privkey.pem; | ||
include /etc/../options-ssl-nginx.conf; | ||
ssl_dhparam /etc/../ssl-dhparams.pem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (documentation): The '/etc/../' path appears to be a placeholder and should be replaced with the actual path
Consider using '/etc/letsencrypt/live/domain.com/' or documenting that this needs to be replaced with the actual certificate path
|
||
With this setup: | ||
|
||
- The URL for accessing the ``eventyay-tickets`` application control panel is ``https://app-test.eventyay.com/tickets/control``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation should be URL agnostic. If we deploy this somewhere else it does not make sense.
|
||
server { | ||
listen 80; | ||
server_name app-test.eventyay.com; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation should be URL agnostic. If we deploy this somewhere else it does not make sense.
|
||
- **/tickets**: | ||
- This location is for the ``eventyay-tickets`` application. | ||
- Requests to ``https://app-test.eventyay.com/tickets/`` are proxied to ``localhost:8455/``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation should be URL agnostic. If we deploy this somewhere else it does not make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comments.
Update documents for how Nginx and URLs path are configured on test/live servers.
Summary by Sourcery
Documentation: