RTECO-1782: JFROG_RUN_NATIVE wins over project config, fix dotnet help, add FlexPack tests - #3699
Closed
bhanurp wants to merge 7 commits into
Closed
RTECO-1782: JFROG_RUN_NATIVE wins over project config, fix dotnet help, add FlexPack tests#3699bhanurp wants to merge 7 commits into
bhanurp wants to merge 7 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 (jfrog#145, jfrog#150, jfrog#151, jfrog#156) and the published-path scenario (jfrog#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>
Contributor
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 jfrog#551).
Contributor
Author
|
Superseded by #3703, which is the same work on a branch in this repository rather than a fork. A PR's head branch cannot be changed after creation, so reopening was the only way to move it. |
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