Automating platform service setup #1374
Conversation
…ture/distributed-demo
|
@8ohamed please rebase the pull request. Thanks. |
…/DTaaS into feature/distributed-demo
prasadtalasila
left a comment
prasadtalasila
left a comment
There was a problem hiding this comment.
@8ohamed Thanks for the pull request. Please see the comments. It seems that the script is only copying the certificates. The goal is to actually run the compose file and run all the certificates.
|
@8ohamed The services in |
Let us take this up in the next PR. |
|
@8ohamed Please consider:
|
|
@prasadtalasila I have resolved the issues. |
There was a problem hiding this comment.
Pull request overview
This PR introduces automation for setting up platform services (RabbitMQ, MongoDB, InfluxDB) by creating a new Python script that handles certificate copying and permission configuration in a cross-platform manner. The work is part of addressing issue #1366 and is still in progress, as noted in the PR description.
Key Changes:
- New
service_setup.pyscript withServicesConfigclass to automate TLS certificate management and permission settings - Addition of UID/GID environment variables for services in the configuration template
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 19 comments.
| File | Description |
|---|---|
deploy/services/script/service_setup.py |
New script implementing automated service setup with methods for certificate copying and permission management for MongoDB, InfluxDB, and RabbitMQ |
deploy/services/config/services.env.template |
Added UID/GID configuration variables for RabbitMQ, InfluxDB, and MongoDB services |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| import platform | ||
| import os | ||
| from pathlib import Path | ||
| import os | ||
| import platform | ||
| import shutil | ||
| import subprocess | ||
| import sys | ||
| from pathlib import Path | ||
| from typing import Tuple | ||
| from dotenv import load_dotenv |
There was a problem hiding this comment.
Missing module-level docstring. The file lacks a module-level docstring explaining the purpose of this script, its usage, and requirements (e.g., need for root privileges, expected environment variables). Add a comprehensive docstring at the top of the file following the existing Python documentation standards in the project.
| certs_target = self.certs_dir / self.host_name | ||
|
|
||
| if self.os_type in ("linux", "darwin"): | ||
| source_dir = Path(f"/etc/letsencrypt/archive/{self.host_name}") |
There was a problem hiding this comment.
Hardcoded certificate path may be a security concern. The path /etc/letsencrypt/archive/{self.host_name} on line 62 is hardcoded and requires root access. This assumes a specific Let's Encrypt installation. Consider making this configurable via an environment variable or configuration file to improve flexibility and allow testing without root privileges.
@8ohamed The script does not need any arguments. For now, please make the script all methods. Please consider the issues raised by sonarcube and copilot. Also do update the README. Thanks. |
|
@prasadtalasila I have applied the suggestions and updated readme, if it looks good then I can start implementing for windows. |
prasadtalasila
left a comment
prasadtalasila
left a comment
There was a problem hiding this comment.
@8ohamed thanks for fixing the previous issues. Please see few more suggestions. The copilot points out few bugs in the code. Please check them as well. Thanks.
|
@prasadtalasila I have applied the suggestions, and the code also handles windows OS now. |
prasadtalasila
left a comment
prasadtalasila
left a comment
There was a problem hiding this comment.
@8ohamed Please see the minor comments.
|
@8ohamed some improvements to text in the README.md. Please copy the raw version of the markdown copy from here onwards. Installation stepsPlease follow the steps outlined here for installation. The script has the following features:
Create Config
InstallInstall Python dependencies before running the script: pip install -r script/requirements.txtRun the installation script cd deploy/services
sudo python3 script/service_setup.pyThe script will:
If any required variable is missing, the script will exit with an error message. This automation reduces manual errors and ensures your service containers have |
| for p in candidates: | ||
| if p != target: |
There was a problem hiding this comment.
The _normalize_cert_candidates method has a potential logic issue. After renaming latest to target (line 122), the subsequent loop (lines 123-125) iterates over the original candidates list and attempts to unlink files where p != target. This will attempt to unlink latest (which no longer exists at its original path after the rename), potentially causing a FileNotFoundError despite the missing_ok=True flag in unlink() at line 124.
The logic should track which file was renamed to avoid attempting to remove it:
def _normalize_cert_candidates(self, prefix: str) -> None:
"""Keep only the latest cert file for a given prefix, rename it, and remove others."""
candidates = list(self.certs["dir"].glob(f"{prefix}*.pem"))
if not candidates:
return
latest = max(candidates, key=lambda p: p.stat().st_mtime)
target = self.certs["dir"] / f"{prefix}.pem"
target.unlink(missing_ok=True)
latest.rename(target)
# Remove all candidates except the one we just renamed
for p in candidates:
if p != latest and p != target:
p.unlink(missing_ok=True)| for p in candidates: | |
| if p != target: | |
| # Remove all candidates except the canonical target (new path) | |
| for p in candidates: | |
| if p.resolve() != target.resolve(): |
|
2 participants
Automating platform service setup
Type of Change
Description
Solving issue #1366, by making methods to adjust permissions (rabbitMQ, MongoDB, InfluxDB), more commits to be made.
Making the methods cross-platform.
Moving the script to Cli.
the PR is not finished yet.