-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: Remove config file #13
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
Conversation
Co-authored-by: Copilot <[email protected]>
Signed-off-by: S3B4SZ17 <[email protected]>
81e1cbc
to
17a1113
Compare
17a1113
to
3438abe
Compare
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.
Pull Request Overview
This PR refactors the application configuration system by removing the YAML config file (app_config.yaml
) and transitioning to environment variable-based configuration. It also updates dependencies to newer versions and simplifies authentication middleware.
- Replaces YAML configuration with environment variable-based AppConfig class
- Updates MCP and FastMCP dependencies to latest versions
- Refactors authentication middleware and tool initialization
Reviewed Changes
Copilot reviewed 31 out of 34 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
utils/app_config.py | Complete rewrite to use environment variables instead of YAML config |
utils/mcp_server.py | Refactored to use class-based server initialization with new AppConfig |
utils/query_helpers.py | Enhanced response parsing with better error handling |
utils/sysdig/legacy_sysdig_api.py | Renamed from OldSysdigApi to LegacySysdigApi with new method |
utils/auth/middleware/auth.py | New middleware implementation for FastMCP framework |
tools/*/tool.py | Updated all tool classes to accept AppConfig and use context-based API access |
charts/sysdig-mcp/* | Updated Helm chart to use new environment variable names |
pyproject.toml | Updated dependency versions including FastMCP and MCP libraries |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
return {} | ||
|
||
|
||
def create_standard_response(results: RESTResponseType, execution_time_ms: float | str, **metadata_kwargs) -> dict: |
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 parameter execution_time_ms
accepts both float | str
but should be consistently typed as float
since it represents a numeric measurement. String values could lead to unexpected behavior in downstream processing.
def create_standard_response(results: RESTResponseType, execution_time_ms: float | str, **metadata_kwargs) -> dict: | |
def create_standard_response(results: RESTResponseType, execution_time_ms: float, **metadata_kwargs) -> dict: |
Copilot uses AI. Check for mistakes.
app_config = get_app_config() | ||
|
||
# Set up logging | ||
logging.basicConfig( | ||
format="%(asctime)s-%(process)d-%(levelname)s- %(message)s", | ||
level=app_config.log_level(), |
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.
Global variable app_config
is initialized at module level but the AppConfig is also passed to the middleware constructor. This creates redundancy and potential inconsistency. Consider removing the global variable and using only the constructor parameter.
app_config = get_app_config() | |
# Set up logging | |
logging.basicConfig( | |
format="%(asctime)s-%(process)d-%(levelname)s- %(message)s", | |
level=app_config.log_level(), | |
# Set up logging | |
logging.basicConfig( | |
format="%(asctime)s-%(process)d-%(levelname)s- %(message)s", | |
level=logging.INFO, # Use a sensible default, or refactor to set this elsewhere with AppConfig |
Copilot uses AI. Check for mistakes.
@@ -122,9 +119,10 @@ def run_sysdig_cli_scanner( | |||
self.check_sysdig_cli_installed() | |||
self.check_env_credentials() | |||
|
|||
tmp_result_file = NamedTemporaryFile(suffix=".json", prefix="sysdig_cli_scanner_", delete_on_close=False) |
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 delete_on_close=False
parameter may not be compatible with older Python versions. Consider using delete=False
instead for better compatibility.
tmp_result_file = NamedTemporaryFile(suffix=".json", prefix="sysdig_cli_scanner_", delete_on_close=False) | |
tmp_result_file = NamedTemporaryFile(suffix=".json", prefix="sysdig_cli_scanner_", delete=False) |
Copilot uses AI. Check for mistakes.
def sysdig_endpoint(self) -> str: | ||
""" | ||
Get the Sysdig endpoint. | ||
|
||
def load_app_config() -> dict: | ||
""" | ||
Load the app config from the YAML file | ||
Raises: | ||
RuntimeError: If no SYSDIG_MCP_API_HOST environment variable is set. | ||
Returns: | ||
str: The Sysdig API host (e.g., "https://us2.app.sysdig.com"). | ||
""" | ||
if f"{ENV_PREFIX}API_HOST" not in os.environ: | ||
raise RuntimeError(f"Variable `{ENV_PREFIX}API_HOST` must be defined.") | ||
|
||
Returns: | ||
dict: The app config loaded from the YAML file | ||
""" | ||
if not check_config_file_exists(): | ||
log.error("Config file does not exist") | ||
return {} | ||
# Load the config file | ||
app_config: dict = {} | ||
log.debug(f"Loading app config from YAML file: {APP_CONFIG_FILE}") | ||
with open(APP_CONFIG_FILE, "r", encoding="utf8") as file: | ||
try: | ||
yaml.add_constructor("!env", env_constructor, Loader=yaml.SafeLoader) | ||
app_config: dict = yaml.safe_load(file) | ||
except Exception as exc: | ||
logging.error(exc) | ||
return app_config | ||
|
||
|
||
def get_app_config() -> dict: | ||
return os.environ[f"{ENV_PREFIX}API_HOST"] | ||
|
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 environment variable name is constructed dynamically multiple times throughout the class. Consider defining these as class constants to avoid repetition and reduce the risk of typos.
def sysdig_endpoint(self) -> str: | |
""" | |
Get the Sysdig endpoint. | |
def load_app_config() -> dict: | |
""" | |
Load the app config from the YAML file | |
Raises: | |
RuntimeError: If no SYSDIG_MCP_API_HOST environment variable is set. | |
Returns: | |
str: The Sysdig API host (e.g., "https://us2.app.sysdig.com"). | |
""" | |
if f"{ENV_PREFIX}API_HOST" not in os.environ: | |
raise RuntimeError(f"Variable `{ENV_PREFIX}API_HOST` must be defined.") | |
Returns: | |
dict: The app config loaded from the YAML file | |
""" | |
if not check_config_file_exists(): | |
log.error("Config file does not exist") | |
return {} | |
# Load the config file | |
app_config: dict = {} | |
log.debug(f"Loading app config from YAML file: {APP_CONFIG_FILE}") | |
with open(APP_CONFIG_FILE, "r", encoding="utf8") as file: | |
try: | |
yaml.add_constructor("!env", env_constructor, Loader=yaml.SafeLoader) | |
app_config: dict = yaml.safe_load(file) | |
except Exception as exc: | |
logging.error(exc) | |
return app_config | |
def get_app_config() -> dict: | |
return os.environ[f"{ENV_PREFIX}API_HOST"] | |
# Environment variable names as class constants | |
API_HOST_ENV = f"{ENV_PREFIX}API_HOST" | |
API_SECURE_TOKEN_ENV = f"{ENV_PREFIX}API_SECURE_TOKEN" | |
TRANSPORT_ENV = f"{ENV_PREFIX}TRANSPORT" | |
LOGLEVEL_ENV = f"{ENV_PREFIX}LOGLEVEL" | |
LISTENING_PORT_ENV = f"{ENV_PREFIX}LISTENING_PORT" | |
LISTENING_HOST_ENV = f"{ENV_PREFIX}LISTENING_HOST" | |
MOUNT_PATH_ENV = f"{ENV_PREFIX}MOUNT_PATH" | |
OAUTH_JWKS_URI_ENV = f"{ENV_PREFIX}OAUTH_JWKS_URI" | |
OAUTH_AUTH_ENDPOINT_ENV = f"{ENV_PREFIX}OAUTH_AUTH_ENDPOINT" | |
OAUTH_TOKEN_ENDPOINT_ENV = f"{ENV_PREFIX}OAUTH_TOKEN_ENDPOINT" | |
if self.API_HOST_ENV not in os.environ: | |
raise RuntimeError(f"Variable `{self.API_HOST_ENV}` must be defined.") | |
return os.environ[self.API_HOST_ENV] |
Copilot uses AI. Check for mistakes.
Signed-off-by: S3B4SZ17 <[email protected]>
3438abe
to
1b3f9d0
Compare
@@ -159,11 +157,11 @@ def run_sysdig_cli_scanner( | |||
"output": output_result + result.stderr.strip(), |
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.
Don't mis stdout and stderr in output
, let the LLM understand which part is stdout and which part is stderr, like:
{
"exit_code": 0,
"stdout": "something failed",
"stderr": "you didn't specify a token, whatever, yada yada",
"exit_codes_explained": "..."
}
if e.returncode in [2, 3]: | ||
log.error(f"Sysdig CLI Scanner encountered an error: {e.stderr.strip()}") | ||
self.log.error(f"Sysdig CLI Scanner encountered an error: {e.stderr.strip()}") | ||
result: dict = { | ||
"error": "Error running Sysdig CLI Scanner", | ||
"exit_code": e.returncode, |
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.
Same thing here
with open(tmp_result_file.name, "r") as output_file: | ||
output_result = output_file.read() | ||
result: dict = { | ||
"exit_code": e.returncode, | ||
"stdout": e.stdout, | ||
"output": output_result, | ||
"exit_codes_explained": self.exit_code_explained, | ||
} |
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.
Same thing here, but it's not consistent, because here we also specify stdout
.
@@ -172,16 +170,18 @@ def run_sysdig_cli_scanner( | |||
} | |||
raise Exception(result) | |||
else: | |||
with open("sysdig_cli_scanner_output.json", "r") as output_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.
I've seen several places where we are writing files, but there's actually no need to write down the contents of the scan, we can just take it from the stdout
from the CLI execution on the fly.
No description provided.