Conversation
35a1316 to
10f9bb0
Compare
There was a problem hiding this comment.
Pull request overview
This PR attempts to update the GitHub Actions workflow to use a CubitPy configuration file instead of the deprecated CUBIT_ROOT environment variable approach. The changes introduce a new cubit_config.yaml file that stores the Cubit installation path and is referenced via a new output parameter cubit_config_file.
Changes:
- Renamed parameter from
cubit-roottocubit_config_filein workflow inputs/outputs - Created new step in cubit_setup to generate a cubit_config.yaml file
- Updated cubit_setup to set cubit_root as an environment variable via GITHUB_ENV
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| .github/workflows/testing_protected.yml | Updated first job to pass cubit_config_file instead of cubit-root to run_tests and cubit_finalize actions |
| .github/actions/run_tests/action.yml | Renamed input parameter and updated description, but incorrectly set CUBIT_ROOT to config file path |
| .github/actions/cubit_setup/action.yml | Added config file creation step, changed output to cubit_config_file, set cubit_root in GITHUB_ENV |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| OMPI_MCA_rmaps_base_oversubscribe: 1 | ||
| run: | | ||
| export CUBIT_ROOT=${{ inputs.cubit-root }} | ||
| export CUBITPY_CONFIG_PATH=${{ inputs.cubitpy_config_file }} |
There was a problem hiding this comment.
This line is incorrectly setting CUBIT_ROOT to the config file path. According to the README (line 376), CUBIT_ROOT should point to the Cubit/Coreform installation path, not a config file. The cubit_setup action already sets the installation path in GITHUB_ENV as 'cubit_root' (line 49 of cubit_setup/action.yml), which should be available as an environment variable here. If CubitPy now requires a config file path in addition to CUBIT_ROOT, it should be exported using a different environment variable name (e.g., CUBITPY_CONFIG_FILE). This line needs to be corrected to either: 1) export CUBIT_ROOT from the environment variable that was set in cubit_setup, or 2) export a different environment variable for the config file path while also ensuring CUBIT_ROOT is properly set.
| export CUBITPY_CONFIG_PATH=${{ inputs.cubitpy_config_file }} | |
| export CUBITPY_CONFIG_PATH=${{ inputs.cubitpy_config_file }} | |
| export CUBIT_ROOT="${{ env.cubit_root }}" |
| uses: ./.github/actions/cubit_finalize | ||
| with: | ||
| cubit-root: ${{ steps.cubit.outputs.cubit_root }} | ||
| cubitpy_config_file: ${{ steps.cubit.outputs.cubitpy_config_file }} |
There was a problem hiding this comment.
The cubit_finalize action is being passed 'cubit_config_file', but the action definition (.github/actions/cubit_finalize/action.yml) has not been updated in this PR and still expects 'cubit-root' as an input parameter. This will cause the finalize step to fail because: 1) the parameter name mismatch means the action won't receive the value, and 2) even if the parameter name were updated, the action needs the actual Cubit installation path (to run rlm_activate), not the config file path. The cubit_finalize action needs to be updated to either accept the installation path via the 'cubit_root' environment variable that was set in cubit_setup, or parse the installation path from the config file.
| cubitpy_config_file: ${{ steps.cubit.outputs.cubitpy_config_file }} | |
| cubit-root: ${{ env.cubit_root }} |
| outputs: | ||
| cubit_root: | ||
| cubitpy_config_file: | ||
| description: "Path to the Cubit installation" |
There was a problem hiding this comment.
The description is incorrect. This output now contains the path to the cubit config file (cubit_config.yaml), not the path to the Cubit installation. The description should be updated to reflect this change.
| description: "Path to the Cubit installation" | |
| description: "Path to the Cubit config file (cubit_config.yaml)" |
|
Does not work as straight forward as I thought, marking as draft for now. |
|
And it did not deactivate Cubit, so no pipelines the next 2 days 😅 |
|
I has a closer look, and let’s place this PR on hold until the changes in imcs-compsim/cubitpy#108 are merged as they will change the CubitPy config again. Another point that came to my mind, @davidrudlstorfer do you have any idea how the changes in this PR did mess up the pipeline? If thought that the pipelines still run on the actions from main (this confused me each time I think about it). |
|
@isteinbrecher I think just the "main" workflow is utilized from main, if you change the action files this could be based on every PR I think |
This should fix the deprecation warning form CubitPy. We will only see if this works, if this is merged.