-
Notifications
You must be signed in to change notification settings - Fork 166
fix: new fields and fixes for Terraform, Cargo, Nuget, Helm, Ansible #1257
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -675,8 +675,6 @@ func virtualTerraformTest(t *testing.T) { | |
| setCacheVirtualRepositoryParams(&tvp.CommonCacheVirtualRepositoryParams, true) | ||
|
|
||
| err = testsUpdateVirtualRepositoryService.Terraform(tvp) | ||
| assert.NoError(t, err, "Failed to update "+repoKey) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't this assertion valid? Is it possible to provide proper comment in the codebase why it was decided to be removed?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed this two lines because there are the same assertions at line 680. |
||
| validateRepoConfig(t, repoKey, tvp) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
| if assert.NoError(t, err, "Failed to update "+repoKey) { | ||
| validateRepoConfig(t, repoKey, tvp) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if let's say this used for updating, are we making sure any empty value sent to the server is not updated in the database?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the implementation in the Artifactory API since I don't have the source for it. But in the client updating local and remote repositories config will result in a call to the method https://github.com/jfrog/jfrog-client-go/blob/master/artifactory/services/repository.go#L28 that will marshal the struct (remove the empty values from the body) and send a POST request. Maybe the POST is implemented like a PATCH on the server and ignore missing fields.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what strange in that
performRequestfunction is that the update will run a POST while the create will do a PUT.