Conversation
Sergio0694
added
the
CI/pipeline 🔬
|
@Sergio0694 this should be what you need, you have different conditions a bit than we do for when things trigger (we only trigger on specific branches for instance). But otherwise I think this is all setup for you. I don't think it'll run until it's merged though. This logic does the following:
This is the same setup we have for WCT. @Arlodotexe feel free to take a quick look in case I missed something (as we have matrix vs. single bits). |
echoix
left a comment
echoix
left a comment
There was a problem hiding this comment.
Some actions here already have newer published versions.
I furthermore reviewed the whole workflow, but not in a dotnet usage point of view, but in a GitHub Actions workflow usage point of view. I have a couple of suggestions, even before using any kind of linter.
| steps: | ||
| - name: Git checkout | ||
| uses: actions/checkout@v4 | ||
|
|
There was a problem hiding this comment.
| with: | |
| persist-credentials: false | |
Since no git push operations are performed, it is a good practice to not have the credentials kept in the git config.
There was a problem hiding this comment.
Good points, we should look into all these in a follow up. Same for the other comments below 🙂
| path: ./packages | ||
|
|
||
| - name: Install Signing Tool | ||
| run: dotnet tool install --tool-path ./tools sign --version 0.9.1-beta.25379.1 |
There was a problem hiding this comment.
Plan a way to keep this tool's version updated. Having it hardcoded to a specific version, beta on top of that, is a little smelly.
In repos that use renovate (not the case here), I would've have seen an env var with that version, and the env var definition having a comment linking back to the source of the project, so updates could be done.
There was a problem hiding this comment.
AFAIK the .NET sign tool only has beta versions so far. @ChrisSfanos is this anything you know about?
|
|
||
| jobs: |
There was a problem hiding this comment.
[nit] Potential improvement: define a global permission key with the common base permissions for all jobs explicitly, or clear the default permissions with {}. Then, in each job, explicitly list the permissions needed and wanted.
This helps when changing the workflow later on, like adding a job, to not have too much permissions.
Also, combine this with a review of the repo's GitHub Actions settings. Some older existing repos have more permissive defaults than newer repos/forks, and these will not limit as much the default permissions to read for contents and packages (usually enough as a default). The change happened a couple years ago
Co-authored-by: Edouard Choinière <[email protected]>
Standardized job and step names in the CI workflow for clarity and consistency. Updated .NET SDK installation to use global.json instead of a hardcoded version. Improved comments and formatting for better maintainability.
|
@Arlodotexe mind looking at the feedback here that could be applied, but also to the WCT repos? |
I looked very shortly yesterday on that other repo to see what was different, and I noticed two actions I didn’t know about. One of them is clearly marked unmaintained and says to look for alternatives |
|
I know the owner of the signing tool doesn't get a lot of time to work on it, but it's been stable and working for many maintainers |
4 participants
This PR moves the workflow for the build pipeline from Azure DevOps to GitHub Actions.