[Setup Command - Maven] - Fix settings.xml corruption and deployment failures #292
Conversation
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
…ang.org/x/crypto, golang.org/x/mod, and several other packages. Modified setup command to use ConfigureArtifactoryRepository instead of ConfigureArtifactoryMirror.
- Add proper cleanup logic to save and restore original GOPROXY values - Add error assertions for all cleanup operations using require.NoError and assert.NoError - Clear GOPROXY after each test case to prevent cross-contamination - Update comments to accurately describe test behavior - Ensure tests don't leave system in modified state after completion Fixes issue where Go configuration tests left GOPROXY set to test values
…into fix-maven-setup
- Introduced a helper function `setupGoProxyCleanup` to encapsulate GOPROXY state management. - Updated tests to use the new cleanup function, ensuring original GOPROXY values are restored after tests. - Improved comments for clarity on test behavior and cleanup process. This change enhances test reliability by preventing environment contamination.
- Move setupGoProxyCleanup() call BEFORE t.Setenv() to capture actual original state - Previous code captured state AFTER clearing environment variable (wrong!) - Now properly preserves and restores the true original GOPROXY configuration - All tests pass with correct cleanup behavior
- Add goProxyEnv constant to eliminate magic string duplication - Change defer setupGoProxyCleanup()() to explicit cleanup := ... defer cleanup() - Makes timing crystal clear: capture happens immediately, only cleanup is deferred - Eliminates all string literal repetition across test functions - Improved readability and maintainability
…into fix-maven-setup
Replace camel-k with etree for Maven settings.xml manipulation to fix: - Duplicate xmlns namespace declarations corrupting settings.xml - mvn clean deploy failures due to missing altDeploymentRepository - Profiles/servers not being added to non-empty settings.xml files Key Changes: - Replace camel-k dependency with beevik/etree (DOM-based XML manipulation) - Implement settings.xml reader that preserves ALL existing user configuration - Add deployment profile with altDeploymentRepository property - Ensure idempotent operations (can run multiple times safely) Implementation Details: - Use DOM tree manipulation instead of struct marshalling - Zero type definitions to maintain (no sync issues with upstream) - Handles namespaces transparently without corruption - Works with both new and existing settings.xml files - Cross-platform path handling (Windows/Linux/macOS) Code Quality: - Comprehensive documentation (package, types, functions, constants) - Robust error handling with context and wrapping - Input validation for required parameters - All strings extracted to named constants (no magic values) - DRY principles with helper functions (62% code reduction) Testing: - 3 test suites covering 8 scenarios - Tests for new file creation and existing file modification - Complex settings.xml test with 30+ preserved fields - Verifies no xmlns duplication or data loss - All authentication types tested (token, basic, anonymous) Dependencies: - Added: github.com/beevik/etree v1.6.0 (lightweight, 20KB) - Removed: github.com/apache/camel-k/v2 (no longer needed) Fixes issues reported in Maven setup where desktop app corrupted settings.xml and failed to configure deployment repository.
sverdlov93
changed the title
Remove error returns from functions that always return nil: - loadSettings() - configureServer() - configureMirror() - configureDeploymentProfile() These functions perform DOM mutations which cannot fail, so error returns were misleading. This fixes the unparam linter warnings.
Resolved conflicts in: - artifactory/commands/setup/setup.go: Added both mvn and ocicontainer imports - go.mod/go.sum: Used main's dependencies and added etree Note: Main branch has dependency issues with jpd package that prevent go mod tidy from running cleanly. This is unrelated to our Maven fix.
The merge accidentally uncommented the replace directives. Main branch has them commented out, so we should too. Only real change: Added etree, removed camel-k dependencies.
omerzi
approved these changes
Oct 26, 2025
Windows Issue: - os.UserHomeDir() uses USERPROFILE on Windows, not HOME - Tests only set HOME, so code wrote to real home, test checked temp dir Fixes: 1. Set both HOME and USERPROFILE in all 3 Maven tests 2. Add NewSettingsXmlManagerWithPath() for custom paths (useful for testing) Now works on Windows, Linux, and macOS ✅
Changes: - Remove local artifactory/commands/mvn/maven.go (implementation moved to cli-core) - Remove artifactory/commands/mvn/testdata/complex-settings.xml - Update setup.go to import from cli-core's maven package - Update setup_test.go to use maven.* constants from cli-core - Remove TestSetupCommand_MavenWithComplexExistingSettings (now in cli-core) - Keep TestSetupCommand_Maven and TestSetupCommand_MavenCorrupted for integration testing - Add Windows compatibility: Set both HOME and USERPROFILE env vars in tests Replace Directive (Temporary for Testing): - Uses local cli-core: /Users/michaelsv/IdeaProjects/cli/jfrog-cli-core - Before merging: Must update to use merged cli-core version after PR is approved Dependencies: - This PR depends on cli-core fix-maven-issues branch being merged first - Once cli-core is merged, remove the replace directive and update go.mod
Changed from local path to remote fork reference: replace github.com/jfrog/jfrog-cli-core/v2 => github.com/sverdlov93/jfrog-cli-core/v2 v2.0.2-0.20251026173539-c1e1fd13ebca This allows CI to build and test the PR without needing local paths. Tests pass ✅
sverdlov93
force-pushed
the
fix-maven-settings-xml-etree
branch
from
2d205f3 to
e3550f6
Compare
…rsion v2.60.1-0.20251026182600-8a8c0428f538. This change ensures compatibility with the latest features and fixes in the remote repository.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🐛 Fixes
Resolves Maven settings.xml corruption and deployment issues:
mvn clean deployfailures (missing altDeploymentRepository)🔧 Solution
Replaced
camel-kdependency withetreefor DOM-based XML manipulation:📊 Changes
Dependencies:
github.com/beevik/etree v1.6.0(20KB)github.com/apache/camel-k/v2Code Quality:
✅ Testing
📈 Impact