Skip to content

Conversation

Eliasj42
Copy link
Contributor

@Eliasj42 Eliasj42 commented Sep 3, 2025

Initial pull request. I'm looking at trimming down comments

@Eliasj42 Eliasj42 requested a review from ScottTodd September 3, 2025 22:36
@ScottTodd
Copy link
Member

cc @jayhawk-commits

Signed-off-by: Joseph <[email protected]>
Comment on lines +3 to +10
on:
push:
branches: [ main ]
pull_request:
branches: [ main ]
schedule:
- cron: "0 1 * * *"
workflow_dispatch:
Copy link
Member

Choose a reason for hiding this comment

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

These triggers are pretty frequent and overlapping.

  • Is a schedule even needed? How about once a week instead of daily?
  • Is pull_request needed? We shouldn't push "latest" images on unreviewed code, but checking that the image builds successfully is useful.

Comment on lines +90 to +93
- name: Docker Push
#if: ${{ github.event_name != 'pull_request' && steps.versions.outputs.PUBLISHED != 'True' }}
run: |
docker push -a ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}
Copy link
Member

Choose a reason for hiding this comment

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

Uncomment? Pull request triggers should not publish (at least without some label marking them as dev builds and not "latest" / "prod"

Comment on lines +22 to +26
build:
runs-on: windows-latest

steps:
- uses: actions/checkout@v4
Copy link
Member

Choose a reason for hiding this comment

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

Prefer to pin runners and actions to specific versions so they don't change outside of our control

Suggested change
build:
runs-on: windows-latest
steps:
- uses: actions/checkout@v4
build:
runs-on: windows-2022
steps:
- uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0

Choose a reason for hiding this comment

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

I remember seeing warnings about changes to Windows runners over the past few months on GitHub. I see the job on this PR failed due to lack of disk space. This could be related. I'll try to dig up info after I finish eating.

Choose a reason for hiding this comment

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

Yep. Looks like they swapped from 2022 to 2025, and the job was last run months ago. Probably is a good idea to be fixed to the 2022 runner.

Copy link
Member

Choose a reason for hiding this comment

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

windows-2025 is also fine with me (if it works)

Choose a reason for hiding this comment

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

I am assuming windows-latest (i.e., 2022) did not run into the disk space error the last time Elias ran this workflow. I see others complaining about disk space with 2025 runners: actions/runner-images#12609

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I didn't see the disc space errors before

# Define the entry point for the docker container.
# This entry point starts the developer command prompt and launches the PowerShell shell.
RUN choco install cmake --version=3.31.0 -y --no-progress --installargs '"ADD_CMAKE_TO_PATH=System"'
RUN choco install python -y --no-progress
Copy link
Member

Choose a reason for hiding this comment

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

We may want to pin the Python version here so it doesn't update when the dockerfile is rebuilt

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