Skip to content

Conversation

@mcakircali
Copy link
Contributor

Description

Contributor Declaration

By opening this pull request, I affirm the following:

  • All authors agree to the Contributor License Agreement.
  • The code follows the project's coding standards.
  • I have performed self-review and added comments where needed.
  • I have added or updated tests to verify that my changes are effective and functional.
  • I have run all existing tests and confirmed they pass.

@mcakircali mcakircali requested a review from iainrussell October 8, 2025 13:08
@mcakircali mcakircali marked this pull request as ready for review October 8, 2025 13:09
Copy link
Collaborator

@recmanj recmanj left a comment

Choose a reason for hiding this comment

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

Added suggestions to keep sha as an input for ci-hpc as well as making sure we fetch the correct config.

@iainrussell what do you think?

owner_repo = "${{ github.repository }}"
owner, repo = owner_repo.split("/")
ref = "${{ github.sha }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will make this step to fetch config from the branch that triggered the workflow run, ignoring ref_name input

owner_repo = "${{ github.repository }}"
owner, repo = owner_repo.split("/")
ref = "${{ github.sha }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might be able to go around this with implementing another step after the checkout step:

      - name: Get checked out SHA
        id: checked_out_sha
        run: echo "sha=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT
Suggested change
ref = os.getenv("CHECKED_OUT_SHA")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not working, can you please check this is correct?

setup:
runs-on: ubuntu-latest
outputs:
matrix: ${{ steps.matrix.outputs.matrix }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
matrix: ${{ steps.matrix.outputs.matrix }}
checked_out_sha: ${{ steps.checked_out_sha.outputs.sha }}

Copy link
Collaborator

Choose a reason for hiding this comment

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

References comment R128

@mcakircali
Copy link
Contributor Author

Added suggestions to keep sha as an input for ci-hpc as well as making sure we fetch the correct config.

@iainrussell what do you think?

@recmanj thanks, I appreciated your suggestions.

our usage of ref_name is to build a branch that is different than the triggering branch. e.g., we use the schedule option that is only triggered on the default branch (develop for us), and we build/deploy another branch(es) (e.g., develop, develop-cpp, nightly).

do we expect using the config from the branch being built or the branch (develop) triggering the build(s)/deploy(s)?

I'm happy in either case but, the config from the triggering branch seems more sensible to me. the configs would be different in rare cases anyways. or at least, I'd prefer 2 jobs in a single config rather than differing configs in different branches.

@mcakircali mcakircali force-pushed the cd-module-add-ref-name branch from 46aecdd to bdfc636 Compare October 10, 2025 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants