-
Notifications
You must be signed in to change notification settings - Fork 52
feat: Publish git-dev nuget package on push to v4 #229
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
base: v4
Are you sure you want to change the base?
Conversation
WalkthroughA new GitHub Actions workflow file named Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GitHub as GitHub Actions
participant NugetJob as nuget job (Windows)
participant GhPagesJob as ghpages job (Ubuntu)
participant SpecterOps as SpecterOps Packages
participant GitHubPages as GitHub Pages
GitHub->>NugetJob: Trigger on push to v4
NugetJob->>NugetJob: Checkout code, setup .NET, restore, test, pack
NugetJob->>NugetJob: Upload coverage report as artifact
NugetJob->>SpecterOps: Publish NuGet packages (using sleet)
NugetJob-->>GhPagesJob: Signal completion
GitHub->>GhPagesJob: Start after nuget job
GhPagesJob->>GhPagesJob: Checkout code, download coverage artifact
GhPagesJob->>GhPagesJob: Build docs with DocFX
GhPagesJob->>GitHubPages: Deploy documentation to gh-pages branch
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
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: 2
🧹 Nitpick comments (1)
.github/workflows/git-dev.yml (1)
36-39
: Remove trailing whitespace for YAML-lint compliance
yamllint
flags lines 36, 39, 48 and 56. Deleting the stray spaces keeps CI green without functional impact.Also applies to: 48-48, 56-56
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/git-dev.yml
(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/git-dev.yml
[warning] 6-6: wrong indentation: expected 6 but found 8
(indentation)
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 39-39: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
.github/workflows/git-dev.yml (2)
26-34
: Verify coverage artifact path exists
dotnet test
alone won’t place results underdocfx/coverage/report/
. If the folder is missing the upload step will hard-fail the job. Confirm that:
- Coverlet / ReportGenerator is invoked by the test projects
- The target directory is created before the upload step
If not, either add the tool invocation or guard the upload with
if: always() && exists('docfx/coverage/report')
.
53-55
: sleet push likely needs an explicit feed configurationUnless the repository root already contains
.config/sleet.json
,sleet push ./pkgs
will error with “Unable to locate configuration”. Consider adding:- sleet push ./pkgs + sleet push --config ./sleet.json ./pkgsand commit the feed config (with URL placeholders, no credentials) to the repo.
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
♻️ Duplicate comments (2)
.github/workflows/git-dev.yml (2)
35-39
: Hard-codedgit-dev
version will collide on every push (repeat feedback).Identical package versions cause
sleet push
to fail on the second attempt. Append the commit SHA (or timestamp) to guarantee uniqueness.- dotnet pack --no-restore -c Release -p:PackageVersion=git-dev -o ./pkgs + dotnet pack --no-restore -c Release \ + -p:PackageVersion=git-dev-${{ github.sha }} \ + -o ./pkgs
3-6
: Workflow triggers contradict PR intent – switchv4
tomain
.The PR description says the package must publish “on push to main,” yet the workflow is wired to
v4
. If this is unintended you will silently never publish frommain
.-on: - push: - branches: - - v4 +on: + push: + branches: + - main # align with PR description
🧹 Nitpick comments (1)
.github/workflows/git-dev.yml (1)
36-56
: Strip trailing whitespace to satisfy YAML linters.Lines 36, 39, 48 and 56 contain trailing spaces flagged by YAML-lint; some CI setups treat this as an error.
Remove the extra blanks to keep the workflow linter-clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/git-dev.yml
(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/git-dev.yml
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 39-39: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
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
♻️ Duplicate comments (1)
.github/workflows/git-dev.yml (1)
3-6
: Trigger branch is stillv4
, contradicting the PR goal of publishing onmain
The PR description states the package should publish on pushes to main, yet the workflow listens to
v4
. This will silently skip runs for the majority of pushes unless developers remember to push to that legacy branch.-on: - push: - branches: - - v4 +on: + push: + branches: + - main # align with PR objective
🧹 Nitpick comments (2)
.github/workflows/git-dev.yml (2)
35-39
: Trailing whitespace – YAMLlint errorsLines 36 and 39 contain stray spaces flagged by YAMLlint. They are harmless to GitHub Actions but break repo linting rules.
- - name: Pack␠ + - name: Pack @@ - dotnet pack --no-restore -c Release -p:PackageVersion=0.0.0-git-dev -o ./pkgs␠ + dotnet pack --no-restore -c Release -p:PackageVersion=0.0.0-git-dev -o ./pkgs
49-56
: Minor: additional trailing space and redundant blank lineLine 56 ends with a space; YAMLlint flags it. While here, the blank line after the command block is unnecessary.
- sleet push ./pkgs --force␠ + sleet push ./pkgs --force
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/git-dev.yml
(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: definitelynotagoblin
PR: SpecterOps/SharpHoundCommon#229
File: .github/workflows/git-dev.yml:35-39
Timestamp: 2025-07-24T21:38:13.462Z
Learning: In the SpecterOps/SharpHoundCommon repository, the team prefers using `sleet push --force` to overwrite git-dev packages rather than creating unique versions with commit SHAs, as they want a single "current state" package for downstream consumers.
.github/workflows/git-dev.yml (1)
Learnt from: definitelynotagoblin
PR: #229
File: .github/workflows/git-dev.yml:35-39
Timestamp: 2025-07-24T21:38:13.462Z
Learning: In the SpecterOps/SharpHoundCommon repository, the team prefers using sleet push --force
to overwrite git-dev packages rather than creating unique versions with commit SHAs, as they want a single "current state" package for downstream consumers.
🪛 YAMLlint (1.37.1)
.github/workflows/git-dev.yml
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 39-39: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/git-dev.yml (1)
26-34
: Verify that the coverage artefact path actually exists
dotnet test
by itself will not createdocfx/coverage/report/
unless a data-collector (e.g., Coverlet) or runsettings file is configured accordingly.
If the directory is missing the upload step will fail the job.Please confirm the path or adjust the test command to generate coverage under that location.
@@ -0,0 +1,80 @@ | |||
name: Publish Git-Dev |
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.
This is just a copy of the existing build action, but specifically to build and push a git-dev nuget package on updates to the default v4
branch.
Description
Right now SH and SHE PRs are tightly coupled to SHC latest stable, and so may not build if there are changes in SHC that haven't yet been published. With more devs on Sharphound now, this lockstep is starting to show us pain. So let's publish a 'git-dev' nuget package to reflect SHC main (that is,
v4
) that SH and SHE pipeline checks can build againstMotivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit