Skip to content
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

Migrate artifactory commands to jfrog-cli-artifactory #1334

Merged
merged 5 commits into from
Jan 28, 2025

Conversation

bhanurp
Copy link
Collaborator

@bhanurp bhanurp commented Jan 17, 2025

  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • All static analysis checks passed.
  • This pull request is on the dev branch.
  • I used gofmt for formatting the code before submitting the pull request.

This pull request depends on the following PR:

@bhanurp bhanurp changed the title Migrate artifactory commands to jfrog-cli-artifactory WIP: Migrate artifactory commands to jfrog-cli-artifactory Jan 17, 2025
@bhanurp bhanurp requested a review from RobiNino January 17, 2025 06:52
@bhanurp bhanurp force-pushed the migrate-artifactory-commands branch from 518b607 to 2c41a1f Compare January 17, 2025 10:57
@bhanurp bhanurp changed the title WIP: Migrate artifactory commands to jfrog-cli-artifactory Migrate artifactory commands to jfrog-cli-artifactory Jan 20, 2025
@EyalDelarea EyalDelarea added the ignore for release Automatically generated release notes label Jan 23, 2025
Copy link
Contributor

@RobiNino RobiNino left a comment

Choose a reason for hiding this comment

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

Nice work!
Leaving most of my review as a comment since it involves unmodified files.

  1. artifactory/commands/gradle - should be deleted.
  2. artifactory/commands/testdata - should be deleted.
  3. Transfer command - I think those should move too, because they belong in the Artifactory repo too, even if they will fall under another team's management. This will also help avoiding the cyclic import which is problematic. Should be moved with the relevant utils in artifactory/commands/utils (precheck, and the transfer named ones)
  4. The tests directory that was migrated should also be deleted. Then, we can also delete the GitHub workflow if it is redundant.

go.mod Outdated Show resolved Hide resolved
@RobiNino RobiNino added breaking change Automatically generated release notes improvement Automatically generated release notes and removed ignore for release Automatically generated release notes labels Jan 23, 2025
@bhanurp
Copy link
Collaborator Author

bhanurp commented Jan 23, 2025

Nice work! Leaving most of my review as a comment since it involves unmodified files.

  1. artifactory/commands/gradle - should be deleted.
  2. artifactory/commands/testdata - should be deleted.
  3. Transfer command - I think those should move too, because they belong in the Artifactory repo too, even if they will fall under another team's management. This will also help avoiding the cyclic import which is problematic. Should be moved with the relevant utils in artifactory/commands/utils (precheck, and the transfer named ones)
  4. The tests directory that was migrated should also be deleted. Then, we can also delete the GitHub workflow if it is redundant.

artifactory/commands/gradle - deleted
tests directory I didnt remove since transfer commands are left over but I have query on that
how is tests/jfrogclicore_test.go helping over go test ./...

@RobiNino
Copy link
Contributor

It provides as a wrapping to all unit tests, so we can set a temporary jfrog home directory and set a logger. Let's currently leave this file as long as there are unit tests in this repository.
Look at it again, the tests workflow might still be required, maybe just modified.

@bhanurp bhanurp requested a review from RobiNino January 23, 2025 15:01
Copy link
Contributor

@RobiNino RobiNino left a comment

Choose a reason for hiding this comment

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

small comment, otherwise LGTM!

go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

👍 Frogbot scanned this pull request and did not find any new security issues.


@bhanurp bhanurp merged commit 4458626 into jfrog:dev Jan 28, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Automatically generated release notes improvement Automatically generated release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants