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

Fixes #523 Add capability to specify proxy for repository replications #524

Merged
merged 2 commits into from
Feb 24, 2022
Merged

Fixes #523 Add capability to specify proxy for repository replications #524

merged 2 commits into from
Feb 24, 2022

Conversation

Guent4
Copy link
Contributor

@Guent4 Guent4 commented Feb 14, 2022

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

I have a use case where I am trying to configure Artifactory replications through Terraform and need to be able to set a proxy for the replication. Couple things to note about the current changes:

  1. The GET JSON response for getting a replication has changed between Artifactory 6.x and 7.x such that this change would only work for 7.x. See Add Proxy and ChartsBaseUrl to Replication and Virtual Repo models atlassian/go-artifactory#33, an old PR, for the change for Artifactory 6.x.
  2. I wanted to added a test for this, but unfortunately was not able to figure out how to dynamically create a proxy via a REST API (couldn't find anything on https://www.jfrog.com/confluence/display/JFROG/Artifactory+REST+API). If you can help point me in the right direction wrt this, I would love to add a test for configuring proxy. I have tested the changes using a GUI/manually created proxy.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Guent4 Guent4 changed the title Add capability to specify proxy for repository replications Fixes 523 Add capability to specify proxy for repository replications Feb 14, 2022
@Guent4 Guent4 changed the title Fixes 523 Add capability to specify proxy for repository replications Fixes #523 Add capability to specify proxy for repository replications Feb 14, 2022
@Guent4
Copy link
Contributor Author

Guent4 commented Feb 14, 2022

I have read the CLA Document and I hereby sign the CLA

Copy link
Contributor

@eyalbe4 eyalbe4 left a comment

Choose a reason for hiding this comment

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

See my inline suggestion.
As for the test, I think that we can avoid adding it in this case.

artifactory/services/utils/replication.go Outdated Show resolved Hide resolved
@eyalbe4 eyalbe4 added the safe to test Approve running integration tests on a pull request label Feb 21, 2022
@Guent4
Copy link
Contributor Author

Guent4 commented Feb 22, 2022

The force push was because I had accidentally committed the fix using a different user and needed to re-write that commit.

@eyalbe4 I noticed that there was an xray test that was failing from your test trigger. Do you have any insights into what might be causing that? Thanks in advance!

@Guent4 Guent4 requested a review from eyalbe4 February 22, 2022 15:07
import "encoding/json"

type ReplicationBody struct {
type replicationBody struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One downside of this change to have two new structs of GetReplicationBody and UpdateReplicationBody rather than overriding the JSONUnmarshal function is that this change is no longer backwards compatible.

Even if I keep choose to keep replicationBody exposed, the function specs in artifactory/services/createreplication.go and artifactory/services/getreplication.go will be changed which are backward incompatible changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Guent4 -
We're mostly okay with breaking compatibility following APO changes. If a public API changed, we make sure to note it in the release notes.

@eyalbe4 eyalbe4 added safe to test Approve running integration tests on a pull request and removed safe to test Approve running integration tests on a pull request labels Feb 23, 2022
@eyalbe4 eyalbe4 merged commit 4e32ce3 into jfrog:dev Feb 24, 2022
@eyalbe4
Copy link
Contributor

eyalbe4 commented Feb 24, 2022

Thanks so much for this important addition @Guent4.
It will be released shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Approve running integration tests on a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants