-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Enable HTTPS use Custom Certificates #7508
Conversation
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.
Thank you for the contribution!
tls.yml
Outdated
tls: | ||
certificates: | ||
- certFile: /certs/your-crt.pem | ||
keyFile: /certs/your-key.pem | ||
stores: | ||
default: | ||
defaultCertificate: | ||
certFile: /certs/your-crt.pem | ||
keyFile: /certs/your-key.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.
I don't think this stub file should be added, because it can't be used as it is.
I would ask you to add instructions in the documentation on how to configure the use of custom certificates, and it seems reasonable to me to describe an example traefik tls configuration file in the documentation in the same place.
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.
I will remove this file and add a document
@echowxsy , could you please add a line into changelog as well about the great contribution? |
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe new documentation file Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
site/content/en/docs/administration/advanced/custom_certificates.md (2)
8-8
: Clarify the role of Traefik as a reverse proxy.The introduction mentions Traefik's default use of Let's Encrypt but could be expanded to briefly explain why Traefik is used as a reverse proxy in CVAT, enhancing understanding for users unfamiliar with Traefik.
22-34
: Add a note to replace placeholder paths in move commands.The documentation does not currently include a note instructing users to replace
/path/to/
with their actual certificate paths. This could lead to confusion. Please add a note to clarify this for users.
- File:
site/content/en/docs/administration/advanced/custom_certificates.md
- Lines: 22-34
Analysis chain
Ensure the path in the move command is accessible.
The commands to move certificates into the
./certs
directory use absolute paths. It's important to ensure that these paths are accessible or to provide a note that users need to replace/path/to/
with the actual path to their certificates.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the provided paths are accessible or if there's a note for users to replace them. # Test: Search for a note about replacing paths in the documentation. rg --type markdown -A 5 $'mv /path/to/'Length of output: 714
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- site/content/en/docs/administration/advanced/custom_certificates.md (1 hunks)
Additional comments not posted (3)
site/content/en/docs/administration/advanced/custom_certificates.md (3)
38-47
: Verify the file path accuracy in the TLS configuration.The
tls.yml
configuration specifies certificate paths as/certs/cert.pem
and/certs/key.pem
. It's crucial to ensure that these paths are correctly mapped in the Docker configuration to avoid runtime errors.
49-68
: Review and validate the Docker Compose configuration changes.The changes to the
docker-compose.https.yml
file include environment variables for Traefik and volume mappings. It's important to ensure that these changes are correctly implemented and do not conflict with existing configurations.Verification successful
No conflicts found between Docker Compose configurations.
The changes in
docker-compose.https.yml
are intended for HTTPS configurations and do not conflict with the existing HTTP configurations indocker-compose.yml
. Ensure that users are aware of the different purposes of these files to avoid accidental simultaneous use. Consider documenting the intended use cases for clarity.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the Docker Compose changes do not conflict with existing configurations. # Test: Search for potential conflicts in the Docker Compose files. rg --type yaml -A 5 $'docker-compose.https.yml'Length of output: 46
Script:
#!/bin/bash # Search for Traefik-related environment variables in the repository to identify potential conflicts. rg 'TRAEFIK_' --type yaml # Find other Docker Compose files in the repository. fd --type f --name 'docker-compose*.yml'Length of output: 1528
Script:
#!/bin/bash # Correct the fd command to list all Docker Compose files. fd --extension yml --exec echo {} # Review the context of the configurations in both files to determine their intended use. rg 'TRAEFIK_' --type yaml -A 5Length of output: 4147
72-76
: Confirm the Docker Compose command's correctness.The command to start CVAT using Docker Compose includes both
docker-compose.yml
anddocker-compose.https.yml
. It's essential to verify that both files are correctly configured to work together without issues.Verification successful
Docker Compose Command Verified
The Docker Compose command in the documentation is correct. The
docker-compose.yml
anddocker-compose.https.yml
files are configured to work together, with the latter adding HTTPS support to the services defined in the former. No issues were found in their configurations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the Docker Compose files are configured to work together. # Test: Search for configurations in both Docker Compose files. rg --type yaml -A 5 $'docker-compose.yml' rg --type yaml -A 5 $'docker-compose.https.yml'Length of output: 87
Script:
#!/bin/bash # Check for the existence of docker-compose.yml and docker-compose.https.yml fd --type f --name 'docker-compose.yml' fd --type f --name 'docker-compose.https.yml'Length of output: 587
Script:
#!/bin/bash # Correctly check for the existence of docker-compose.yml and docker-compose.https.yml fd 'docker-compose.yml' fd 'docker-compose.https.yml'Length of output: 97
Script:
#!/bin/bash # Inspect the contents of docker-compose.yml and docker-compose.https.yml cat docker-compose.yml cat docker-compose.https.ymlLength of output: 12441
site/content/en/docs/administration/advanced/custom_certificates.md
Outdated
Show resolved
Hide resolved
site/content/en/docs/administration/advanced/custom_certificates.md
Outdated
Show resolved
Hide resolved
site/content/en/docs/administration/advanced/custom_certificates.md
Outdated
Show resolved
Hide resolved
site/content/en/docs/administration/advanced/custom_certificates.md
Outdated
Show resolved
Hide resolved
…es.md Co-authored-by: Andrey Zhavoronkov <[email protected]>
…es.md Co-authored-by: Andrey Zhavoronkov <[email protected]>
…es.md Co-authored-by: Andrey Zhavoronkov <[email protected]>
…es.md Co-authored-by: Andrey Zhavoronkov <[email protected]>
@echowxsy Thanks for the contribution! |
Could you please fix Remark linter warning?
|
Done,I just finished my vacation. so sorry too late. |
Quality Gate passedIssues Measures |
Thank you! |
Motivation and context
fix #4767
my docker-compose.https.yml:
Login page:
Traefik Dashboard:
Checklist
develop
branch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Documentation