Implemented Artifactory upload and download handling #30

eeasaa01 · 2024-11-05T10:09:55Z

Implemented Artifactory Cache Integration

Changes

Added GRABIT_ARTIFACTORY_TOKEN support
Implemented cache download priority
Added fallback to source URLs
Added integrity validation
Added unit tests for cache functionality
Added integration tests
Fixed logging levels
Added proper error handling

Testing Done

Added TestGrabitRequirements for comprehensive testing
Tested token validation
Tested cache download and fallback
Tested NO_CACHE_UPLOAD
Tested error handling
All tests passing

Changes Made Based on Feedback

Fixed authentication error handling
Adjusted log levels for cache operations
Improved error messages
Added proper validation at each step
Added comprehensive test coverage

Checklist

I have read the contributing guidelines
Functionality is documented
All code style checks pass
New code contribution is covered by automated tests
All new and existing tests pass

jsquyres

There appears to be quite a bit of functionality and many changes that are not directly related to the Artifactory / caching functionality, which makes this PR a bit hard to read and review.

Also, can we get the writeup of what we decided would be the first cut of this functionality? Without that, @rabadin can't properly evaluate this code.

I assume that you notice that the CI tests are failing, too. You'll want to make test before you push again and make sure everything passes.

Finally, let's talk in the next meeting about how to add some unit tests for this functionality.

cmd/add.go

cmd/download.go

internal/lock.go

internal/resource.go

…sful uploads

…ion failures

cmd/add.go

jsquyres · 2024-11-07T14:03:22Z

Note that this PR now has a (simple) conflict that has to be fixed. Github won't run the CI on a PR that has conflicts, which is why there's currently no CI results here.

…tion

- Added TestGrabitRequirements - Tested token validation - Tested cache download and fallback - Tested NO_CACHE_UPLOAD - Tested error handling - All tests passing

Implemented Artifactory upload and download handling - Added GRABIT_ARTIFACTORY_TOKEN support - Implemented cache download priority - Added fallback to source URL - Added validation at each step - Improved error handling and logging - Integrated cache upload functionality#

…sful uploads

…ion failures

pick 0ea343f Implemented requested changes and added tests for Artifactory integration pick 082c98b Added comprehensive tests for Artifactory cache functionality

…tion

- Added TestGrabitRequirements - Tested token validation - Tested cache download and fallback - Tested NO_CACHE_UPLOAD - Tested error handling - All tests passing

jsquyres

Need to remove the a640.... binary.

FWIW: Using git add -a or git commit -a is dangerous. It is much safer to git add ...list of files... or git add -p to select precisely which things you want to add to the staging area.

.gitignore

cmd/add.go

internal/resource.go

- Removed binary file a640986bc2 - Updated .gitignore to exclude binaries and artifacts - Modified cmd/add.go to properly handle cache - Updated internal/resource.go for cache functionality - Improved error handling per review

- Modified function to take token as parameter - Removed duplicate os.Getenv call - Improved error messages - Fixed as per code review feedback

- Changed NO_CACHE_UPLOAD to GRABIT_NO_CACHE_UPLOAD - Following naming convention for environment variables - Addressing code review feedback

- Fix empty if block in downloadFromSource - Update variable naming for consistency - Move algorithm extraction - Improve code organization

- Replace http.NewRequest with requests library - Remove unnecessary comment - Keep same functionality with safer implementation - Follow code review feedback

internal/resource.go

internal/hash.go

rabadin · 2024-11-13T08:56:20Z

internal/lock.go

 	}
+	defer file.Close()


Looks like it yes. A small one since this would be GCed but still.

jsquyres · 2024-11-19T02:25:21Z

It looks like there were some bad merges that were not fixed properly before committing. For example, I see a full Open MPI tarball added in 4033f12. Word to the wise: you should avoid using git add -a or git commit -a. It is far to easy to commit files that you do not intend.

Additionally, make test fails to compile the tests (it looks like there's some git conflict markers that were left in the source files). This is one reason that the CI is failing. In general, you should probably ensure that grabit builds properly and the tests are passing before pushing up to github.

internal/lock.go

internal/resource_test.go

internal/resource.go

internal/lock_test.go

internal/artifactory_cache_test.go

grsbit

internal/lock.go

rabadin · 2024-11-21T21:03:29Z

internal/lock.go

+	artifactoryURL := fmt.Sprintf("%s/%s", cacheURL, hexHash)
+
+	// Create upload request
+	req, err := http.NewRequest("PUT", artifactoryURL, bytes.NewReader(fileContent))


As discussed, let's use github.com/carlmjohnson/requests for non-test http interactions, like the rest of the code does.

rabadin · 2024-11-22T08:30:12Z

internal/lock.go

 		}
 	}
 	l.conf.Resource = newStatements
 }

+func deleteCache(url, token string) error {
+	req, err := http.NewRequest("DELETE", url, nil)


Same remark here, let use github.com/carlmjohnson/requests for http interactions.

internal/lock.go

rabadin · 2024-11-27T09:38:41Z

@amdurh05 Also, you have a random 11M binary file in here.

jsquyres · 2024-11-27T13:18:14Z

cmd/download_test.go

@@ -112,9 +112,9 @@ func TestRunDownloadFailsIntegrityTest(t *testing.T) {
 	content := `abcdef`
 	port := test.TestHttpHandler(content, t)
 	testfilepath := test.TmpFile(t, fmt.Sprintf(`
-	[[Resource]]


Let's revert these whitespace changes. There's no need for these changes for the Artifactory functionality.

We did not make any changes in this part of the file. I believe these whitespace modifications were introduced by Mr. Raphael in a prior commit.

jsquyres · 2024-11-27T14:17:00Z

cmd/download_test.go

This file is poorly named, but let's not rename it as part of this PR -- let's rename it in a separate PR.

rabadin · 2024-11-27T15:25:25Z

internal/artifactory_cache_test.go

+func TestDownloadWithArtifactoryCache(t *testing.T) {
+	t.Run("NO_CACHE_UPLOAD", func(t *testing.T) {
+		// Turn on NO_CACHE_UPLOAD setting
+		os.Setenv("NO_CACHE_UPLOAD", "1")


Use https://pkg.go.dev/testing#B.Setenv instead. This guarantees that it gets restored after the test runs.

Implemented Artifactory upload and download handling

1675df4

jsquyres requested changes Nov 5, 2024

View reviewed changes

eeasaa01 added 3 commits November 5, 2024 16:08

Fixed authentication error and adjusted log level to debug for succes…

0a01f2e

…sful uploads

Conditional ARTIFACTORY_TOKEN check and removed spurious comment

412a3aa

Restored COMMENT_PREFIX and improved error logging for resource creat…

825c3ea

…ion failures

jsquyres reviewed Nov 6, 2024

View reviewed changes

cmd/add.go Outdated Show resolved Hide resolved

cmd/add.go Outdated Show resolved Hide resolved

cmd/add.go Outdated Show resolved Hide resolved

cmd/add.go Outdated Show resolved Hide resolved

Fixed Artifactory integration and addressed PR feedback

d6229c9

eeasaa01 and others added 10 commits November 8, 2024 00:14

Implemented requested changes and added tests for Artifactory integra…

0ea343f

…tion

Added comprehensive tests for Artifactory cache functionality

082c98b

- Added TestGrabitRequirements - Tested token validation - Tested cache download and fallback - Tested NO_CACHE_UPLOAD - Tested error handling - All tests passing

Updating delete function to also delete artifactory cache

a484aa3

Fixed authentication error and adjusted log level to debug for succes…

e98d7af

…sful uploads

Conditional ARTIFACTORY_TOKEN check and removed spurious comment

503e529

Restored COMMENT_PREFIX and improved error logging for resource creat…

f5f034e

…ion failures

Fixed Artifactory integration and addressed PR feedback

410b85b

pick 0ea343f Implemented requested changes and added tests for Artifactory integration pick 082c98b Added comprehensive tests for Artifactory cache functionality

Implemented requested changes and added tests for Artifactory integra…

4033f12

…tion

Added comprehensive tests for Artifactory cache functionality

c389fb2

- Added TestGrabitRequirements - Tested token validation - Tested cache download and fallback - Tested NO_CACHE_UPLOAD - Tested error handling - All tests passing

eeasaa01 force-pushed the feature/artifactory-upload branch from 082c98b to c389fb2 Compare November 12, 2024 20:38

Cleanup: Remove binary files and add gitignore

2ab9448

jsquyres requested changes Nov 12, 2024

View reviewed changes

eeasaa01 added 7 commits November 12, 2024 23:49

Fix: Code review feedback and cleanup

2090e45

- Removed binary file a640986bc2 - Updated .gitignore to exclude binaries and artifacts - Modified cmd/add.go to properly handle cache - Updated internal/resource.go for cache functionality - Improved error handling per review

Fix: Pass token as parameter to uploadToArtifactory

1ee74b5

- Modified function to take token as parameter - Removed duplicate os.Getenv call - Improved error messages - Fixed as per code review feedback

Fix: Prefix NO_CACHE_UPLOAD with GRABIT_

f0676ab

- Changed NO_CACHE_UPLOAD to GRABIT_NO_CACHE_UPLOAD - Following naming convention for environment variables - Addressing code review feedback

Fix: Code improvements and cleanup

5d9a7c8

- Fix empty if block in downloadFromSource - Update variable naming for consistency - Move algorithm extraction - Improve code organization

Fix: Use requests library for cache upload

513e420

- Replace http.NewRequest with requests library - Remove unnecessary comment - Keep same functionality with safer implementation - Follow code review feedback

Update cmd/add.go with feedback implementation

8a41679

Update cmd/cache_test.go with cache download tests

f2e8c42

rabadin reviewed Nov 13, 2024

View reviewed changes

eeasaa01 marked this pull request as draft November 13, 2024 13:34

eeasaa01 and others added 5 commits November 18, 2024 15:27

Update resource.go

5d77339

Delete a640986bc257389dd379886fdae6264c8cfa56bc98b71ce3ae3dfbd8ce61dbe3

1acba07

Delete openmpi-5.0.5.tar.bz2

6794f82

Replace old project files with new ones

1273d45

Fix typo in filename: dowload_test.go -> download_test.go

e5aefe8

eeasaa01 marked this pull request as ready for review November 18, 2024 21:00

Fix merge conflict

2b68e57

rabadin requested changes Nov 20, 2024

View reviewed changes

readding artifactory add functionality

f7f3445

jsquyres reviewed Nov 21, 2024

View reviewed changes

grsbit Outdated Show resolved Hide resolved

rabadin reviewed Nov 21, 2024

View reviewed changes

internal/lock.go Outdated Show resolved Hide resolved

rabadin reviewed Nov 21, 2024

View reviewed changes

amdurh05 added 2 commits November 21, 2024 23:21

Updating delete function and changing Artifactory filename to SRI format

f93213b

Fix bug in AddResource

8301d12

rabadin requested changes Nov 22, 2024

View reviewed changes

eeasaa01 and others added 4 commits November 26, 2024 12:54

Fix Artifactory download logic and tests for resource cache behavior

534d546

Update resource.go

9344f52

Update lock.go

01d5f3e

Update artifactory_cache_test.go

1f5cf14

rabadin requested changes Nov 27, 2024

View reviewed changes

internal/lock.go Outdated Show resolved Hide resolved

eeasaa01 added 2 commits November 27, 2024 07:03

Update lock.go

2d597ba

Delete grsbit

738ec4b

jsquyres reviewed Nov 27, 2024

View reviewed changes

eeasaa01 added 3 commits November 27, 2024 08:43

refactor: replace http client with requests library in lock.go

ba91254

fix: update logic in lock.go

fb7c978

WIP: Save local changes before pulling

79d9e43

jsquyres reviewed Nov 27, 2024

View reviewed changes

cmd/download_test.go

Copy link

Contributor

jsquyres Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is poorly named, but let's not rename it as part of this PR -- let's rename it in a separate PR.

rabadin reviewed Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Artifactory upload and download handling #30

Implemented Artifactory upload and download handling #30

eeasaa01 commented Nov 5, 2024 •

edited

Loading

jsquyres left a comment •

edited

Loading

jsquyres commented Nov 7, 2024

jsquyres left a comment

rabadin Nov 13, 2024

jsquyres commented Nov 19, 2024

rabadin Nov 21, 2024

rabadin Nov 22, 2024

rabadin commented Nov 27, 2024

jsquyres Nov 27, 2024

eeasaa01 Nov 27, 2024

jsquyres Nov 27, 2024

rabadin Nov 27, 2024 •

edited

Loading

Implemented Artifactory upload and download handling #30

Are you sure you want to change the base?

Implemented Artifactory upload and download handling #30

Conversation

eeasaa01 commented Nov 5, 2024 • edited Loading

Implemented Artifactory Cache Integration

Changes

Testing Done

Changes Made Based on Feedback

Checklist

jsquyres left a comment • edited Loading

Choose a reason for hiding this comment

jsquyres commented Nov 7, 2024

jsquyres left a comment

Choose a reason for hiding this comment

rabadin Nov 13, 2024

Choose a reason for hiding this comment

jsquyres commented Nov 19, 2024

rabadin Nov 21, 2024

Choose a reason for hiding this comment

rabadin Nov 22, 2024

Choose a reason for hiding this comment

rabadin commented Nov 27, 2024

jsquyres Nov 27, 2024

Choose a reason for hiding this comment

eeasaa01 Nov 27, 2024

Choose a reason for hiding this comment

jsquyres Nov 27, 2024

Choose a reason for hiding this comment

rabadin Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

eeasaa01 commented Nov 5, 2024 •

edited

Loading

jsquyres left a comment •

edited

Loading

rabadin Nov 27, 2024 •

edited

Loading