Skip to content

Conversation

@shaohuzhang1
Copy link
Contributor

fix: Set the startup environment variables

@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Nov 6, 2025

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Nov 6, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

env.setdefault('C_FORCE_ROOT', '1')
kwargs = {
'cwd': self.cwd,
'stderr': self.log_file,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are several areas where the code can be improved:

  1. Environment Variables: It's good practice to avoid using absolute paths like settings.APPS_DIR in environment variables unless necessary. Instead, consider setting them up at deployment time through a configuration file.

  2. Security: Using 'C_FORCE_ROOT' is not recommended as it bypasses security measures that prevent root execution of scripts. If running the script on a server under user permissions, ensure that the script is executed with appropriate permissions rather than forcing root mode.

  3. Default Environment Setup: The use of default values for environment variables such as PYTHONOPTIMIZE and ANSIBLE_FORCE_COLOR doesn't seem relevant unless these features are explicitly needed and have been tested thoroughly.

Here's an optimized version of the code snippet:

# Import required modules
import subprocess
from .celery_base import CeleryBaseService

__all__ = ['CeleryDefaultService']

class CeleryDefaultService(CeleryBaseService):
    def __init__(self, **kwargs):
        super().__init__(**kwargs)
        self.env = {}

    def open_subprocess(self):
        # Set common environment variables
        self.set_common_environment_variables()

    def set_common_environment_variables(self):
        env_copy = os.environ.copy()
        env_copy['SERVER_NAME'] = 'celery'

        # Use relative paths instead of full paths from settings if possible
        app_directories = ','.join(settings.APP_DIRECTORIES)  # Assuming you have APP_DIRECTORIES in settings
        env_copy['PYTHONPATH'] = app_directories

        if os.getuid() != 0:
            env_copy['LC_ALL'] = 'C.UTF-8'
            env_copy['ANSIBLE_FORCE_COLOR'] = 'True'
            env_copy.setdefault('PYTHONOPTIMIZE', '1')

        return environ_copy

Key Changes:

  • Use Relative Paths: Avoid hardcoded paths like settings.APPS_DIR. This makes the application more portable across different environments.
  • Common Environment Setup: Extracted into a separate method (set_common_environment_variables) for better modularity and easier maintenance.
  • Remove Unnecessary Checks: Removed the condition checking for root_uid since it was not used anywhere else.

Make sure to replace settings.APP_DIRECTORIES with the actual path(s) your applications require during runtime.

@shaohuzhang1 shaohuzhang1 merged commit 8a0a807 into v2 Nov 6, 2025
3 of 6 checks passed
@shaohuzhang1 shaohuzhang1 deleted the pr@v2@fix_celery branch November 6, 2025 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants