RTECO-1782: JFROG_RUN_NATIVE wins over project config, fix dotnet help, add FlexPack tests - #3703
Open
bhanurp wants to merge 19 commits into
Open
RTECO-1782: JFROG_RUN_NATIVE wins over project config, fix dotnet help, add FlexPack tests#3703bhanurp wants to merge 19 commits into
bhanurp wants to merge 19 commits into
Conversation
…help
The gate was `ShouldRunNative(configFilePath) && !configExists`, so any
.jfrog/projects/{dotnet,nuget}.yaml left in a project forced the legacy path
even with JFROG_RUN_NATIVE=true. Nothing was logged. Worse, the legacy path
does not recognise the native-only flags, so it forwarded them to MSBuild and
the run died with:
MSBUILD : error MSB1001: Unknown switch.
Switch: --repo-resolve
which names an MSBuild flag and gives no hint that a YAML file two directories
down is the cause. If the stale config happened to be valid, the build instead
resolved from whatever repository that file named rather than the
--repo-resolve on the command line - wrong-repo resolution with no diagnostic
at all.
The environment variable now takes precedence and the config file is reported
and ignored. Applied to both DotnetCmd and NugetCmd, which had the identical
gate and the identical failure. Verified that the legacy path still works when
JFROG_RUN_NATIVE is unset, and that a project with neither a config nor the
variable still gets the original "run jf dotnet-config first" error.
The help text was the other half of the problem. It listed 'jf dotnet-config'
as a prerequisite - "must be run first" - so a user following the CLI's own
documentation created exactly the file that disabled FlexPack. That command is
out of scope for FlexPack per the spec, yet remained the documented happy path.
It is now described as optional under JFROG_RUN_NATIVE, for both dotnet and
nuget.
Also corrected the dotnet sub-command list, which claimed "(restore, build,
pack, push)". There is no 'jf dotnet push'; the real command is the two-token
'jf dotnet nuget push', which was fully implemented in getNugetCommandName but
documented nowhere. The list now reads restore, build, publish, pack, add and
nuget push, with an example and an explicit note that plain 'jf dotnet push' is
not a command.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds dotnet_native_test.go, covering the dotnet CLI toolchain on the FlexPack code path. That combination had no coverage: nuget_test.go covers both toolchains on the legacy path, nuget_native_test.go covers nuget.exe on FlexPack, and this fills the empty quadrant. Derived from the Confluence test plan "Dotnet Flexpack support in jfrog-cli test plan" (RTFACT 2729476103). 114 tests against its 186 scenarios: all 40 P0, all 106 P1, 38 of 40 P2. Every test names the scenarios it covers, so coverage is auditable against the plan rather than asserted. 26 tests are t.Skip with a stated reason, in three groups. Known product gaps: the curation-on-failure hook is not wired for dotnet, and --scan is accepted on push but stripped. Missing fixtures: .fsproj, .vbproj and .slnx projects, signed packages, packages over 100 MB. Infrastructure this harness does not provision: build promotion, Xray scan, release bundles, CI provider simulation, self-signed TLS, and proxying - each skip names the nuget_native_test.go helper to reuse when porting those groups. Infrastructure mirrors nuget_native_test.go and reuses its shared helpers (createNugetProject, getFlexPackItemProps, buildTestNupkg, allowInsecureConnectionForFlexPackTests, createThrowawayRepo, initNugetTest, cleanTestsHomeEnv) rather than duplicating them. Runs under the existing -test.nuget flag, which is what provisions the NuGet repositories these tests share; there is no separate dotnet flag. Four P0 auth scenarios (#145, #150, #151, #156) and the published-path scenario (#13) are asserted AS IMPLEMENTED rather than as specified, and the file header lists each divergence with both readings. The plan states JFrog CLI injects no temp nuget.config and exports nothing to the child environment; the implementation does both, and packages land flat rather than under <Name>/<Version>. nuget_native_test.go already carries the same divergence list for nuget.exe. These need reconciling with the spec owner - the tests pin current behaviour so that a change in either direction shows up as a failure. Compile-verified and vet-clean; not yet run against a live Artifactory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI surfaced three classes of issue that could not be reproduced locally, because the gosec build here fails with an internal type error under this Go toolchain. G703 (path traversal, 11 sites): the .csproj rewrites are annotated with the repo-standard justification. Every path is built from the test its own temp project directory, never from external input, matching the existing precedent in conan_test.go and nuget_native_test.go. G122 and nilerr (one site): the sha512-corruption walk both wrote inside the filepath.Walk callback, which gosec flags as a symlink TOCTOU race, and returned nil when the walk itself errored. Sidecar paths are now collected during the walk and rewritten after it returns, and the walk error is propagated. This is also simply more correct: a failure to traverse the cache should fail the test rather than silently corrupting nothing. Verified with the exact commands CI runs: gosec reports 0 issues in both repos, and golangci-lint with the Static Check linter set reports 0 issues. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI ran the suite for the first time and 65 of the failures traced to one line.
createNugetProject returns a path relative to the working directory, and
enterDotnetProject used it for two things that both require an absolute one:
NUGET_PACKAGES, which NuGet rejects outright -
error : NUGET_PACKAGES must contain an absolute path out/reference/.packages
that accounted for 46 failures.
And file paths built by callers AFTER the helper chdirs into the project. A
relative projectPath then resolves against the project directory itself, so
writing out/reference/nuget.config from inside out/reference looked for
out/reference/out/reference/nuget.config - 19 more failures.
Resolving the path once at the top of the helper fixes both.
The remaining failure is a real product gap rather than a test defect, and is
now asserted as such. Credential injection appends --configfile to the end of
the argument list, which places it after a user double-dash separator;
everything past that separator goes to MSBuild, which rejects the switch:
MSBUILD : error MSB1001: Unknown switch.
Switch: --configfile
The injected flag needs to precede the separator. The subtest asserts the
current failure with that explanation, so it will start passing by itself once
the ordering is fixed rather than being silently skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The subtest documented the MSB1001 failure as a known gap. That gap is fixed in jfrog-cli-artifactory (insertBeforeSeparator), so the case asserts success again and the comment records why the ordering matters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docker_test.go: initDockerBuildTest set JFROG_RUN_NATIVE=true before calling initNativeDockerWithArtTest, which t.Skip()s when '-test.docker=true' is absent. t.Skip runs runtime.Goexit, so the helper never returned and the caller's 'defer cleanup()' was never registered - the variable stayed set for the rest of the test binary. docker_test.go sorts before nuget_test.go, so every legacy 'jf nuget' test silently ran through the FlexPack path and TestNugetResolve's requestedBy assertions failed. Run the skip check first and restore via t.Cleanup so the value is undone even if a later helper skips or fails. dotnet_native_test.go, TestDotnetFlexPackTransitiveDepsResolved: counted a dependency as transitive when a RequestedBy path had len > 1. Since stripModuleFromRequestedBy drops the trailing module ID, a one-level transitive dep is ["bootstrap:4.0.0"] and a direct dep is ["reference:1.0.0"] - both length 1 - and this fixture's graph is only one level deep, so the count was always zero. Compare path[0] against the enclosing module ID instead. dotnet_native_test.go, TestDotnetFlexPackPrivateAssetsScope: the test appended a second PackageReference for Newtonsoft.Json carrying PrivateAssets="all", but the fixture already declares that package. The SDK deduplicates duplicate PackageReference items (NU1504) and keeps the first, so suppressParent never reached project.assets.json and "compile" was correct. Mark the existing reference private instead, guard the fixture literal, add the missing ordinary-reference half of the scenario, and fail if neither dependency is found rather than passing vacuously.
Explicit --source pushes (NugetApiKeyEnvVar, ApiKeyFlagOverridesEnv,
UserSourceOverridesConfig, CiSecretBackedApiKeyPush): NuGet 6.8+ refuses an HTTP
source unless allowInsecureConnections is set on a *configured* source, and jf
writes no nuget.config of its own when no --repo is given (NuGetFlexPackCommand
only injects one under `repo != ""`), so nothing supplied that permission and
the pushes died with "NuGet requires HTTPS sources" against the plain-HTTP test
Artifactory. Add insecureSourceConfigFile, which declares the same URL with
allowInsecureConnections and is passed via --configfile. It carries no
credential, so each test still proves what it was written to prove.
LocalRepoPublishAndResolve: resolution is pinned to a local repo holding only
the package just pushed, but the consumer project kept the simple-dotnet
fixture's four unrelated references, which that repo cannot serve - restore
failed NU1101 before the round trip was exercised. Replace the fixture's
ItemGroup instead of appending to it.
DependencyRangeResolvesConcreteVersion: was passing vacuously. It appended a
second PackageReference for a package the fixture already declares, so the SDK
collapsed the duplicate (NU1504) and the version range was discarded; the
assertions then held trivially. Turn the existing reference into a range, and
assert the range resolves to its lowest applicable version (13.0.0) rather than
merely that no dependency id contains a bracket.
LockedModeInconsistency: drifted the project by appending a duplicate reference,
which the SDK collapsed the same way - so the graph still matched the lock file,
locked mode restored happily and NU1004 never fired. Bump the existing
reference's version instead.
CentralPackageManagement: CPM is project-wide, so every PackageReference still
carrying a Version is NU1008 and fails the restore outright. Strip the versions
from all four fixture references and move them into Directory.Packages.props,
pinning Newtonsoft.Json centrally to a different version so the assertion proves
the version came from CPM.
BuildFlagsIncomplete: the CLI rejects a half-specified build-name/build-number
pair rather than silently skipping collection; assert the error instead of
NoError, and keep the check that no build-info is produced.
PushDefault: 'dotnet nuget push' publishes the .snupkg alongside the .nupkg, and
build-info types it "snupkg". Type each artifact by extension instead of
demanding "nupkg" for both, keeping the never-zip regression guard.
BceCapturesEnv and the two nuget_native_test.go 'bag' call sites: 'bce'/'bag'
are local commands, and the credential flags this runner appends land after the
positional args, where Go's flag parser has already stopped - so they were
counted as arguments ("Wrong number of arguments (4)"). The two nuget tests were
skipping over this while blaming a missing git repository. Use
WithoutCredentials(), as every other bce/bag call site in this suite does.
FlagPassthrough/double-dash-separator is left asserting the fixed behaviour; it
passes once go.mod resolves a jfrog-cli-artifactory carrying
insertBeforeSeparator (jfrog-cli-artifactory PR #551).
…O-1782 work Both were pinned to commits predating this change set, so CI was exercising the old behaviour: the artifactory pin still wrote <packageSourceCredentials> into the temp nuget.config, and carried neither insertBeforeSeparator (the --configfile vs "--" separator fix) nor the NuGetPackageSourceCredentials_<source> environment channel; the build-info-go pin lacked the externally-resolved dependency warning. build-info-go b325d34 -> c457602 (jfrog/build-info-go#424) jfrog-cli-artifactory 4c19791 -> 151d331 (jfrog/jfrog-cli-artifactory#552) Both branches live in their own repositories, so these resolve without a replace directive. Re-pin to the merge commits once those two PRs land.
…st faults Bump build-info-go to f06729d, which stores a pushed symbol package flat at the repository root as "<id>.<version>.snupkg" instead of "symbolpackage/<id>.<version>.nupkg". Verified natively against a live Artifactory, with no jf involved: 'dotnet nuget push' sends the .nupkg to /api/nuget/<repo> and the .snupkg to /api/nuget/v3/<repo>/symbols, and AQL then reports both flat at the root, the symbol package keeping its own extension, plus an extracted .pdb under .symbols/. Pointing the same push at a V2 source uploads no symbol package at all, so no configuration reaches the V2 /symbolpackage endpoint whose layout the old code assumed. The CI log shows both toolchains using the V3 /symbols endpoint 300 times and /symbolpackage never. That path was not only used for symbol lookups: stampBuildProperties resolves every pushed artifact by exact path before stamping and errors when one is missing, so any push carrying a sibling .snupkg failed in full - taking build-info collection and the exit code with it - though both files had uploaded successfully. Twelve of the sixteen failures in the previous run trace back to it, including BuildPromote, PushBuildInfoAndProperties and ReleaseBundleFromNugetBuild, which are not about symbols at all. Seven assertions in nuget_native_test.go encoded the same wrong layout, so they had been passing only because test and product agreed with each other. They now look for the flat path. Four failures were unrelated: DependencyRangeResolvesConcreteVersion expected [13.0.0, 14.0.0) to resolve to 13.0.0. Newtonsoft.Json has no 13.0.0 release, so the lowest version within the range is 13.0.1. LocalRepoPublishAndResolve resolved through the local repo holding only the package under test, and failed NU1101 on Microsoft.NETCore.App.Ref: the fixture's target framework has no targeting pack in the installed SDK, so restore must fetch one. Resolve through the virtual repo, whose deployment target is that same local repo, so the round trip is still what is proven. DetailedSummary passed --detailed-summary, which is not in the Dotnet or Nuget flag sets, so it reached 'dotnet nuget push' as a second package path and failed with "File does not exist". Native mode does not support detailed summary; assert the current behaviour and describe what to replace it with if that changes. ArtifactoryUnreachableNoFallback restored successfully against an unreachable host because earlier tests had already populated the shared global packages folder. Point NUGET_PACKAGES at an empty per-test directory so the restore has to reach the network.
…-summary assertion Bump build-info-go to db34798, which records a .snupkg found beside a pushed .nupkg even though it never appears on the command line. Both native clients discover and upload it themselves, so jf was collecting one artifact where two files had just been created: the symbol package ended up in the repository with no build-info entry and no build.name/build.number, invisible to anything selecting artifacts by build properties. Property stamping needed no change - it walks whatever the collector returns - so the three symbol tests that asserted on build-info contents or item properties now have something to find. TestDotnetFlexPackDetailedSummary asserted the wrong string. dotnet prints "error: File does not exist (--detailed-summary=true)" to the console, but jf wraps the child's exit status rather than propagating its message, so the Go error reads "dotnet nuget push failed: exit status 1". Assert on failure rather than on text the error never carries.
The test ran 'dotnet pack --include-symbols' and checked only that the command exited zero, while its comment claimed the snapshot diff collects both packages - so the behaviour it names was never verified. Assert that build-info carries the .snupkg alongside the .nupkg.
PackIncludeSymbols asserted a .snupkg, but 'dotnet pack --include-symbols' leaves SymbolPackageFormat at its default and emits '<id>.<version>.symbols.nupkg'; .snupkg needs -p:SymbolPackageFormat=snupkg. Verified by running the command directly. The switch also tested '.nupkg' before '.symbols.nupkg', so the symbols package was being counted as the primary one. Accept either format and order the suffix tests so the longer one wins. SymbolRoundTrip used getFlexPackItemProps as an existence check, which cannot distinguish a missing item from one carrying no properties. Its push names no build, so jf stamps nothing, and a .snupkg never receives the nuget.id/nuget.version that Artifactory attaches when indexing a primary package - so the file round-tripped correctly and was still reported missing. Check existence over HTTP instead, via a new assertArtifactExists helper documenting why properties are the wrong signal.
shouldRunNuGetFlexPack called artutils.ShouldRunNative("") and then needed a
comment explaining that the empty string makes the config-path condition vacuous.
flexpack.IsFlexPackEnabled() says the same thing without the sentinel or the
comment, and cli/cli.go already calls it that way.
Dependency bumps carry the cleanup from the sibling PRs: build-info-go b4cc3ad
(scaffolding left by the snupkg path change) and jfrog-cli-artifactory ef644fe
(unreachable code the native-publish change left behind).
The help text told users package restoration is routed via Artifactory for restore, build, publish, pack and add without qualification. Two of those were not true: publish and pack silently ignored --repo-resolve (fixed in jfrog-cli-artifactory 2d943f6, now bumped here), and 'dotnet add package' cannot be routed at all because the .NET SDK gives it no config-file option. Say which sub-commands honour the flag, name the workaround for add, and state where build-info dependencies versus artifacts come from. Dependency bumps: jfrog-cli-artifactory 2d943f6 (config-file collision, unconfigured server, empty credentials, dotnet add, pack/publish implicit restore, pack output directories) and build-info-go 07bdda7 (requestedBy traversal hang, directory named like a package, glob pattern error).
Eight tests were passing without exercising their stated behaviour. MultiTargetFrameworkGraph never multi-targeted: its replacer named netstandard2.0 and net8.0 while the fixture declares net7.0, so the rewrite was a silent no-op and the test passed on an ordinary single-TFM restore. Match whatever TFM the fixture declares, require the rewrite to change the file, and check project.assets.json carries a target for each framework before asserting on the graph. GlobalPackagesFolderFromConfig asserted a folder was absent that NUGET_PACKAGES - set by enterDotnetProject - already guaranteed would be absent, so the result did not depend on whose config NuGet read. Drop that variable for the test and assert both directions: the folder appears when jf injects nothing, and does not once --repo-resolve makes jf supply its own config file. StampWithBadTokenPreservesPushExit named a server profile that does not exist, so it failed during server-id resolution before the push ran - the opposite of its premise that stamping fails after a successful push. Create a profile that exists with an invalid token, give the push working credentials, and assert the package really did reach the repository. StampFailureSurfaces had the same problem via an HTTP --source the push rejected outright; it now pushes successfully and fails only on the missing stamp target. PushWildcardGlob built each package in its own t.TempDir(), so the glob could match only one file and the wildcard was never exercised; both packages now share a directory and both must appear in build-info and in the repository. IdCasingFromNuspec, SymbolOnlyPush and VirtualRepoPushConvention each asserted only inside a loop or an if that may never be entered. Added the missing guards, and VirtualRepoPushConvention now asserts the resolved repo positively rather than merely that it is not the virtual one. LegacySymbolsFormat asserted 'not zip', which cannot fail since the type mapper returns only nupkg or snupkg; it now pins the real mapping. docker_test.go: initNativeDockerWithArtTest returned its home-dir restore to the caller, so a t.Skip or failed require between that point and the caller's defer would leak JFROG_CLI_HOME_DIR into every later test - the same shape as the JFROG_RUN_NATIVE leak already fixed in initDockerBuildTest. Register it with t.Cleanup instead.
gosec G101 matches identifiers containing 'token', so badTokenServerId - a server-id string, not a secret - was reported as a potential hardcoded credential and failed the Go-Sec job. Rename it and its value; nothing about the test changes. Worth recording: gosec skips _test.go files unless -tests is passed, which the CI scanner does and a plain 'gosec ./...' does not. Local verification of test-file findings must use 'gosec -tests ./...'.
…ually runs The push succeeded and the post-push step reported 'artifact info collected' with no error, so the assertion failed. stampBuildProperties returns early when no deploy repo is set - an anonymous push has no JFrog target to stamp - and the test passed --source without --repo, so the failure it is named for was never reachable. Adding --repo makes the post-push work resolve the repository through the broken-token server and fail there, which is the behaviour under test: an error after a successful upload must surface rather than be swallowed. The push still succeeds independently of that server: --source with --api-key and the user's own --configfile means jf injects nothing, so the invalid token never reaches the upload.
Contributor
bhanurp
requested review from
a team,
agrasth,
fluxxBot,
itsmeleela,
reshmifrog and
udaykb2
and removed request for
a team
September 9, 2026 07:18
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two commits: a behaviour fix in the FlexPack gate with the help text it depends on, and the dotnet FlexPack integration suite.
1.
JFROG_RUN_NATIVEnow wins over a project config fileThe gate was:
so any
.jfrog/projects/{dotnet,nuget}.yamlin a project forced the legacy path even withJFROG_RUN_NATIVE=true. Nothing was logged.Worse, the legacy path doesn't recognise the native-only flags, so it forwarded them to MSBuild:
— which names an MSBuild flag and gives no hint that a YAML file two directories down is the cause. And if the stale config happened to be valid, the build silently resolved from whatever repository that file named rather than the
--repo-resolveon the command line. Wrong-repo resolution, no diagnostic.Now the env var takes precedence and the config is reported and ignored:
Applied to both
DotnetCmdandNugetCmd— identical gate, identical failure.Regression-verified, all three paths:
JFROG_RUN_NATIVE=true+ config presentjf dotnet-configfirst" error ✅The help text was the other half
jf dotnet --helplistedjf dotnet-configas a prerequisite — "must be run first". So a user following the CLI's own documentation created exactly the file that disabled FlexPack. That command is out of scope for FlexPack per the spec, yet remained the documented happy path. Now described as optional underJFROG_RUN_NATIVE, for bothdotnetandnuget.Also corrected the sub-command list, which claimed
(restore, build, pack, push). There is nojf dotnet push— the real command is the two-tokenjf dotnet nuget push, fully implemented ingetNugetCommandNamebut documented nowhere. Now: restore, build, publish, pack, add, andnuget push, with an example and an explicit note.This closes two open bug-hunt tickets about
-hnot showing supported args andjf dotnet nugetbeing undocumented.2. dotnet FlexPack integration tests
Adds
dotnet_native_test.go. That combination had no coverage — the empty quadrant:nuget_test.gonuget_native_test.godotnet_native_test.goDerived from the Confluence test plan (RTFACT 2729476103). 114 tests against its 186 scenarios: all 40 P0, all 106 P1, 38/40 P2. Every test names the scenarios it covers, so coverage is auditable against the plan rather than asserted.
26 are
t.Skipwith a stated reason, in three groups:--scanaccepted on push but stripped.fsproj/.vbproj/.slnx, signed packages, >100 MB packagesnuget_native_test.gohelper to reuse when porting.Infrastructure mirrors
nuget_native_test.goand reuses its shared helpers rather than duplicating them. Runs under the existing-test.nugetflag, which is what provisions the NuGet repositories these tests share — there is no separate dotnet flag.nuget_native_test.gois untouched.Five P0 scenarios contradict the implementation. Following the precedent already set in
nuget_native_test.go, the tests pin current behaviour, and the file header lists each divergence with both readings so a change in either direction shows up as a failure:nuget.configis writtenNuGetPackageSourceCredentials_*<repo>/<Name>/<Version>/<file>.nupkg<repo>/<file>.nupkgThese need reconciling with the spec owner — either the spec or the implementation is wrong, and this PR doesn't decide which.
Testing
gofmt,go build ./...,go vet,golangci-lint— all cleanMerge order
Last of three RTECO-1782 PRs. Both dependencies must be merged and bumped first —
TestDotnetFlexPackRequestedByHasNoRedundantPathsasserts the build-info-go dedupe behaviour.🤖 Generated with Claude Code