-
Notifications
You must be signed in to change notification settings - Fork 28
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
ci: refactor msbuild workflow #136
Conversation
WalkthroughThe Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/msbuild.yml (1 hunks)
Additional comments not posted (6)
.github/workflows/msbuild.yml (6)
4-10
: LGTM! Expanded workflow triggers improve CI/CD.The new workflow triggers ensure that the workflow runs on pull requests and manual dispatches, which is useful for verifying PRs and manually checking the build state.
12-14
: LGTM! Concurrency settings enhance workflow efficiency.The updated concurrency settings prevent multiple workflows from running simultaneously for the same ref, which is efficient and prevents resource wastage.
24-24
: LGTM! Updated action versions ensure latest features and fixes.Updating the versions for
checkout
,setup-msbuild
, andupload-artifact
actions ensures that the workflow uses the latest features and bug fixes provided by these actions.Also applies to: 29-29, 76-76
16-18
: LGTM! Secure handling of sensitive data with environment variables.Introducing the
CERTIFICATE_FILE
environment variable sourced from GitHub secrets is a secure way to handle sensitive data.Also applies to: 53-54
32-35
: LGTM! Improved readability and simplified workflow.The reformatting of script execution blocks improves readability, and the removal of redundant scripts simplifies the workflow.
Also applies to: 64-70
Line range hint
75-80
:
LGTM! Updated artifact upload step with improved readability.Updating the artifact upload step to use the latest version of the
upload-artifact
action ensures that the workflow benefits from the latest features and fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/msbuild.yml (1 hunks)
Additional comments not posted (7)
.github/workflows/msbuild.yml (7)
4-10
: Approved: Enhanced workflow triggers.The addition of
pull_request
,push
, andworkflow_dispatch
events improves the workflow's flexibility and ensures comprehensive CI/CD coverage.
12-14
: Approved: Concurrency settings.The concurrency settings ensure that only one workflow runs for each ref, canceling in-progress workflows if new changes are pushed. This optimizes resource usage and prevents redundant builds.
16-18
: Approved: Streamlined environment variables.The environment variables section has been shortened while maintaining equivalence, improving readability without altering functionality.
24-24
: Approved: Updated GitHub Actions versions.Updating to newer versions of
actions/checkout
,microsoft/setup-msbuild
, andactions/upload-artifact
ensures compatibility with newer features or fixes.Also applies to: 29-29, 76-76
32-39
: Approved: Reformatted VCPKG packages restoration and extraction.The script for restoring and extracting VCPKG packages has been reformatted for better readability, improving maintainability without altering functionality.
63-70
: Approved: Reformatted MSBuild command.The MSBuild command has been reformatted for better readability, improving maintainability without altering functionality.
75-79
: Approved: Updated artifact upload step.Updating the artifact upload step to use version 4 ensures compatibility with newer features or fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/msbuild.yml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/msbuild.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/msbuild.yml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/msbuild.yml
63aa00d
to
ccbbcce
Compare
62b5e91
to
f9db83c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/msbuild.yml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/msbuild.yml
After this is merged, I plan on working on a way to cache the third party builds as that part is quite time consuming (~25 minutes). |
Looks like i need to tweak something else. I have it working in my repo, so should be close. |
f9db83c
to
ef96562
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/msbuild.yml (1 hunks)
Additional comments not posted (10)
.github/workflows/msbuild.yml (10)
4-10
: Event triggers updated successfully.The addition of specific
pull_request
types and restriction to themaster
branch forpush
events, along withworkflow_dispatch
, enhances control and flexibility.
12-14
: Concurrency settings optimized.The concurrency configuration prevents overlapping runs, reducing resource waste and ensuring builds are up-to-date.
16-18
: Environment variables simplified.The streamlined
env
section maintains functionality while enhancing readability.
24-29
: Action versions updated.Upgrading to newer versions of actions like
actions/checkout@v4
andmicrosoft/setup-msbuild@v2
is a good practice for leveraging improvements and features.
32-45
: VCPKG package handling improved.The rewritten steps for handling VCPKG packages using multi-line commands enhance readability and maintainability.
52-95
: Certificate handling for forks implemented.The new step for generating a self-signed certificate and using the GITHUB_TOKEN for password creation enhances security and adapts to different repository contexts.
97-109
: Local certificate handling secured.The use of
CERTIFICATE_FILE
for local builds ensures secure handling of sensitive information.
113-132
: Build step robustness enhanced.The conditional logic for handling certificate passwords dynamically improves the robustness and flexibility of the build process.
134-137
: Certificate cleanup ensured.The cleanup step guarantees the removal of sensitive files, enhancing security.
138-142
: Artifact upload updated.Using
actions/upload-artifact@v4
ensures compatibility and benefits from any improvements in the action.
|
I think I've fixed this. |
Thank you as always! |
This PR makes some changes the msbuild.yml workflow. I am happy to describe any changes in further detail if needed.
pull_request
allows you to verify if PRs have successful builds and/or download the artifacts for testing. This is limited to opened, synchronized (when new commits are pushed to the PR branch), and re-opened events.push
is limited to only themaster
branch.workflow_dispatch
allows you to run this workflow manually if you ever need to check the build state.concurrency
changes will ensure only a single workflow will run for each ref. This means if a PR contributor pushes new changes to their branch, it will cancel the in process workflow and trigger a new run.env
section was shortened, but equivalent to the previousshell: pwsh
as this is the default for Windows runnersworking-directory: ${{env.GITHUB_WORKSPACE}}
as this is the default for all job stepsThere is a repository setting to configure how github workflows run from outside collaborators. The options are:
No matter which option you select, PRs from forks will never have access to secrets. There are none in your workflow currently, so this is just FYI.
P.S. In another PR, I would like to add dependabot to help you keep actions and git submodules updated automatically. Unfortunately, it does not have support for vcpkg (see: dependabot/dependabot-core#2027)
Summary by CodeRabbit
New Features
Improvements