Skip to content

feat: Replace compose env vars with parameters #4284

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

Merged
merged 37 commits into from
Jul 24, 2025

Conversation

hpohekar
Copy link
Collaborator

@hpohekar hpohekar commented Jul 15, 2025

This pull request replaces the use of environment variables for selecting Docker Compose or Podman Compose with explicit parameters in the codebase and updates associated documentation accordingly. The changes improve clarity and maintainability by introducing a ComposeConfig class to manage these configurations.

Key Changes:

Environment Variable Replacement

  • Removed the PYFLUENT_USE_DOCKER_COMPOSE and PYFLUENT_USE_PODMAN_COMPOSE environment variables, replacing them with parameters use_docker_compose and use_podman_compose in relevant methods and constructors [1] [2] [3].

Codebase Refactoring

  • Introduced the ComposeConfig class to encapsulate Docker Compose and Podman Compose configurations, replacing direct environment variable checks throughout the codebase [1] [2].
  • Updated the ComposeBasedLauncher and other related classes to use the new ComposeConfig for determining the compose commands and configurations [1] [2].

Documentation Updates

  • Updated user guides and examples to reflect the removal of environment variables and the introduction of new parameters for specifying compose options [1] [2] [3].
  • Removed references to deprecated environment variables in the documentation and added instructions for using the new parameters [1] [2].

Deprecation Notices

  • Added deprecation warnings for the removed environment variables in the doc/deprecated_pyfluent_apis.py file, specifying their replacements.

Changelog

  • Added a changelog entry summarizing the replacement of environment variables with parameters.

@hpohekar hpohekar linked an issue Jul 15, 2025 that may be closed by this pull request
@github-actions github-actions bot added documentation Documentation related (improving, adding, etc) maintenance General maintenance of the repo (libraries, cicd, etc) CI/CD new feature Request or proposal for a new feature labels Jul 15, 2025
@hpohekar hpohekar marked this pull request as ready for review July 21, 2025 14:20
@hpohekar
Copy link
Collaborator Author

hpohekar commented Jul 21, 2025

Podman workflow is working fine. - https://github.com/ansys/pyfluent/actions/runs/16489890949/job/46622027333

@hpohekar hpohekar marked this pull request as draft July 21, 2025 14:33
@github-actions github-actions bot removed the CI/CD label Jul 22, 2025
@github-actions github-actions bot added the CI/CD Related to CI/CD label Jul 22, 2025
@hpohekar hpohekar marked this pull request as ready for review July 22, 2025 10:10
@seanpearsonuk
Copy link
Collaborator

@hpohekar

General comments on the current approach:

  1. Accessing the environment at various points to infer program state and make logical decisions in the code is dangerous. The environment can change underneath the logic, which makes it unreliable. Instead, we should read the relevant environment variables once at the highest-level construction point and ignore them after that.

  2. Environment variables should always play second fiddle to arguments explicitly set by the user. This is a good general principle: argument values are in a narrower scope (more localized) and should therefore take precedence. This is especially important here since these environment variables are now deprecated.

Let’s make sure the logic reflects this hierarchy and avoids reading the environment more than once.

@hpohekar
Copy link
Collaborator Author

@hpohekar

General comments on the current approach:

  1. Accessing the environment at various points to infer program state and make logical decisions in the code is dangerous. The environment can change underneath the logic, which makes it unreliable. Instead, we should read the relevant environment variables once at the highest-level construction point and ignore them after that.
  2. Environment variables should always play second fiddle to arguments explicitly set by the user. This is a good general principle: argument values are in a narrower scope (more localized) and should therefore take precedence. This is especially important here since these environment variables are now deprecated.

Let’s make sure the logic reflects this hierarchy and avoids reading the environment more than once.

@seanpearsonuk Yes, I totally agree. Done. Thank you.

@seanpearsonuk
Copy link
Collaborator

@hpohekar
General comments on the current approach:

  1. Accessing the environment at various points to infer program state and make logical decisions in the code is dangerous. The environment can change underneath the logic, which makes it unreliable. Instead, we should read the relevant environment variables once at the highest-level construction point and ignore them after that.
  2. Environment variables should always play second fiddle to arguments explicitly set by the user. This is a good general principle: argument values are in a narrower scope (more localized) and should therefore take precedence. This is especially important here since these environment variables are now deprecated.

Let’s make sure the logic reflects this hierarchy and avoids reading the environment more than once.

@seanpearsonuk Yes, I totally agree. Done. Thank you.

@hpohekar It's still the same in the version I'm seeing. I might be missing a commit.

@hpohekar hpohekar marked this pull request as draft July 22, 2025 12:40
@hpohekar hpohekar marked this pull request as ready for review July 23, 2025 13:10
@hpohekar
Copy link
Collaborator Author

@hpohekar
General comments on the current approach:

  1. Accessing the environment at various points to infer program state and make logical decisions in the code is dangerous. The environment can change underneath the logic, which makes it unreliable. Instead, we should read the relevant environment variables once at the highest-level construction point and ignore them after that.
  2. Environment variables should always play second fiddle to arguments explicitly set by the user. This is a good general principle: argument values are in a narrower scope (more localized) and should therefore take precedence. This is especially important here since these environment variables are now deprecated.

Let’s make sure the logic reflects this hierarchy and avoids reading the environment more than once.

@seanpearsonuk Yes, I totally agree. Done. Thank you.

@hpohekar It's still the same in the version I'm seeing. I might be missing a commit.

@seanpearsonuk We have updated the code as per our latest discussion.

ComposeConfig - https://github.com/ansys/pyfluent/pull/4284/files#diff-9ffea1d210c7640c063600a138d17ff230ba5a0e88c2af306721694bb9f8759e

Instantiated only once - https://github.com/ansys/pyfluent/pull/4284/files#diff-6388218efa10000a687f0828690e9072e2752a00c66a5cccb0fa2e737344a112

We are passing ComposeConfig at all places wherever it is necessary.

Thanks a lot for this suggestion, the workflow is straightforward now.

@hpohekar hpohekar marked this pull request as draft July 23, 2025 15:40
@hpohekar hpohekar marked this pull request as ready for review July 24, 2025 08:14
@hpohekar hpohekar merged commit e1ffefe into main Jul 24, 2025
34 checks passed
@hpohekar hpohekar deleted the feat/make_compose_env_vars_parameter branch July 24, 2025 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Related to CI/CD documentation Documentation related (improving, adding, etc) maintenance General maintenance of the repo (libraries, cicd, etc) new feature Request or proposal for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace compose env vars with parameters
4 participants