Feature: xray watch #211

josh-barker-coles · 2020-10-14T02:37:47Z

Description:

This PR creates:

an Xray Manager that uses a separate endpoint
a function to retrieve the Xray Version - GetXrayVersion
functions to Create, Get, Update and Delete an Xray Watch
- CreateXrayWatch
- GetXrayWatch
- UpdateXrayWatch
- DeleteXrayWatch

The Xray Watch functions support the configuration of Repositories and Builds.

There is a placeholder method to configure Bundles.
I can't implement the support for Bundles because of the limits on the free trial for artifactory.

Related Issues

Resolves #202

github-actions · 2020-10-14T02:38:04Z

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

josh-barker-coles · 2020-10-14T03:41:13Z

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

josh-barker-coles · 2020-10-14T03:42:41Z

recheckcla

josh-barker-coles · 2020-10-15T23:06:33Z

Hi @eyalbe4, it would be good to get your review on this.

Cheers

eyalbe4 · 2020-10-16T03:46:42Z

Sure @josh-barker-coles.
I'll get to reviewing this soon.

eyalbe4

I haven't gone through everything yet. Releasing what I have so far.

eyalbe4 · 2020-10-16T07:48:50Z

tests/jfrogclient_test.go

 	if *DistUrl != "" {
 		createDistributionManager()
 	}
+	if *XrayUrl != "" {


Now that this has been added as part of the InitArtifactoryServiceManager function, we should probably rename the function from InitArtifactoryServiceManager to InitServiceManagers.

eyalbe4 · 2020-10-16T08:03:59Z

tests/utils.go

@@ -158,6 +167,14 @@ func createDistributionManager() {
 	testsBundleDeleteRemoteService.DistDetails = distDetails
 }

+func createXrayManager() {
+	xrayDetails := GetXrayDetails()
+	client, err := rthttpclient.ArtifactoryClientBuilder().SetServiceDetails(&xrayDetails).Build()


Now that ArtifactoryClientBuilder also needs to be used for Xray, we should rename it to HttpClientBuilder.
We should also move the httpclient package, which is currently located under the artifactory directory, to be one level up, next to the artifactory dir.
rthttpclient should be renamed to httpclient, or if the the httpclient is already taken as an alias in the file, maybe jfroghttpclient.

Hey @eyalbe4,

I understand why you want to move that functionality, but I've noticed a few things.

In the root directory, there already is a generic httpclient package, which ArtifactoryHttpClient depends on.

The Distribution manager is also depend on the ArtifactoryClientBuilder

This sounds like a big change that should go in a separate PR, as it would effect many files under artifactory/*, distribution/*, tests/* and xray/*.

As the httpclient directory & package is already taken, what did you want to do?

Cheers

Got it @josh-barker-coles. We'll create an issue for this and handle this in a follow up PR.

eyalbe4 · 2020-10-16T08:07:28Z

tests/utils.go

@@ -158,6 +167,14 @@ func createDistributionManager() {
 	testsBundleDeleteRemoteService.DistDetails = distDetails
 }

+func createXrayManager() {


For consistency, this function should be renamed to * createXrayVersionManager*.

eyalbe4 · 2020-10-16T16:45:39Z

xray/services/utils/xraywatchbody.go

+	AssignedPolicies []XrayAssignedPolicy      `json:"assigned_policies,omitempty"`
+}
+
+// structs that aren't exported


What does "exported" mean?
BTW, our convention is is to start all comments with an upper case letter.

@eyalbe4
If a struct or function starts with an upper case, it's available in the package to be imported somewhere else.
However, if a struct or function starts with a lower case, it's internal and cannot be imported elsewhere.

As these structs are used for internally transforming the parameters to the payload, I opted to not export them.
What would you prefer?

eyalbe4 · 2020-10-16T16:47:55Z

xray/services/utils/xraywatchbody.go

+			payloadBody.ProjectResources.Resources = append(payloadBody.ProjectResources.Resources, repo)
+		}
+	case "":
+		// empty is fine


// empty is fine --> // Empty is fine

eyalbe4 · 2020-10-16T16:56:35Z

xray/services/utils/xraywatchbody.go

+	case "":
+		// empty is fine
+	default:
+		return errors.New("Invalid Repository Type. Must be " + string(WatchRepositoriesAll) + " or " + string(WatchRepositoriesByName))


We should wrap this error with errorutils.CheckError(err)..
In this case, the above line should:

errorutils.CheckError(errors.New("Invalid Repository Type. Must be " + string(WatchRepositoriesAll) + " or " + string(WatchRepositoriesByName)))

errorutils.CheckError is used to wrap the error when it is created by the code, or if it is created by the code of an external package, the error is wrapped the first time it is returned in the code. This approach allows us to track the source code generating the error if needed.
I noticed there's one more place in this file where this si needed.

eyalbe4 · 2020-10-16T17:02:25Z

xray/services/utils/xraywatchbody.go

+	return nil
+}
+
+func configureBundles(payloadBody *XrayWatchBody, params XrayWatchParams) error {


We prefer not to have placeholder code in the form of empty methods. I suggest you to the description of the CreateBody function a comment saying that bundles are currently not supported. This disclaimer should also be added to the API description in the README.

eyalbe4

Done reviewing everything.
Thanks so much for adding this!
See my inline comments.

eyalbe4 · 2020-10-17T08:52:08Z

tests/xraywatch_test.go

+	assert.Equal(t, params.Description, targetConfig.Description)
+	assert.Equal(t, params.Active, targetConfig.Active)
+	assert.Equal(t, params.Policies, targetConfig.Policies)
+	return


Redundant return statement.

eyalbe4 · 2020-10-17T09:00:52Z

tests/xraywatch_test.go

+	artUtils.SetContentType("application/json", &xrayHTTPDetails.Headers)
+	client, err := httpclient.ClientBuilder().Build()
+	if err != nil {
+		return nil


Should probably be fixed to return err.

eyalbe4 · 2020-10-17T09:03:11Z

tests/xraywatch_test.go

+	resp, _, err := client.SendDelete(xrayDetails.GetUrl()+"api/v2/policies/"+policyName, nil, xrayHTTPDetails)
+	if err != nil || resp.StatusCode != http.StatusOK {
+		return errors.New("failed to delete policy " + resp.Status)
+	}


I suggest changing the above to:

if err != nil { return error } if resp.StatusCode != http.StatusOK { return errors.New("failed to delete policy " + resp.Status) }

This also applies to other checks in this file.

eyalbe4 · 2020-10-17T09:08:01Z

xray/services/xraywatch.go

+	"net/http"
+
+	rthttpclient "github.com/jfrog/jfrog-client-go/artifactory/httpclient"
+	// "github.com/jfrog/jfrog-client-go/artifactory/services/utils"


Let's remove this commented line.

eyalbe4 · 2020-10-17T09:12:51Z

xray/services/xraywatch.go

+)
+
+// XrayWatchService defines the http client and xray details
+type XrayWatchService struct {


Ket's rename the struct to WatchService and this file to watch.go.
Let's also rename NewXrayWatchService to NewWatchService.

@eyalbe4

Most, if not all, structs have a prefix of Xray.
Did you want them all to be renamed without Xray?

Hi @eyalbe4, I've renamed all structs & functions that included Xray.

eyalbe4 · 2020-10-17T09:31:11Z

xray/services/xraywatch.go

+	return nil
+}
+
+// Get retrieves the details about an Xray watch by name


// Get retrieves the details about an Xray watch by name --> // Get retrieves the details of an Xray watch by its name.

eyalbe4 · 2020-10-17T09:45:09Z

xray/manager.go

+}
+
+// GetXrayVersion will return the xray version
+func (sm *XrayServicesManager) GetXrayVersion() (string, error) {


Let's rename GetXrayVersion() to GetVersion(), CreateXrayWatch to CreateWatch and so on.

eyalbe4 · 2020-10-17T09:48:14Z

README.md

+    - [Creating an Xray Watch](#creating-an-xray-watch)
+    - [Get an Xray Watch](#get-an-xray-watch)
+    - [Update an Xray Watch](#update-an-xray-watch)
+    - [Delete an Xray Watch](#delete-an-xray-watch)


Let's also include the new version API in the README.

eyalbe4 · 2020-10-17T09:50:15Z

README.md

+```go
+serviceConfig, err := config.NewConfigBuilder().
+    SetServiceDetails(rtDetails).


rtDetails --> xrayDetails

eyalbe4 · 2020-10-17T09:51:16Z

README.md

+    SetCertificatesPath(certPath).
+    SetThreads(threads).
+    SetDryRun(false).


Let'e remove SetThreads andSetDryRun

josh-barker-coles · 2020-10-19T22:31:49Z

Hey @eyalbe4, thanks for the feedback. I'll start working on it this week.

josh-barker-coles · 2020-10-22T01:18:24Z

Hi @eyalbe4, I'll fix the README conflict after we've resolved all the above discussions.

josh-barker-coles · 2020-11-03T22:23:30Z

Hi @eyalbe4, what else is remaining for this PR?
I'd like to complete this so I can work with someone on jfrog/terraform-provider-artifactory#10

Cheers,

Josh

eyalbe4

Thanks so much for this awesome contribution @josh-barker-coles!
Please go ahead and resolve the conflict, and I'll merge this PR.

…holder function

josh-barker-coles · 2020-11-08T23:40:53Z

You're welcome @eyalbe4.

I've just rebased & resolved the README.md issue.

yahavi · 2021-02-07T07:58:02Z

@josh-barker-coles
FYI, the APIs for Xray Watches in the Go Client are going to change in the next version - We are removing the HTTP response from the returned values.
We think that returning the HTTP response is "too low level" for such APIs. If the API failed to create the watch, an error is returned.

Before the change:

func (xws *WatchService) Delete(watchName string) (*http.Response, error)
func (xws *WatchService) Create(params utils.WatchParams) (*http.Response, error)
func (xws *WatchService) Update(params utils.WatchParams) (*http.Response, error)
func (xws *WatchService) Get(watchName string) (watchResp *utils.WatchParams, resp *http.Response, err error)

After that changes:

func (xws *WatchService) Delete(watchName string) error
func (xws *WatchService) Create(params utils.WatchParams) error
func (xws *WatchService) Update(params utils.WatchParams) error
func (xws *WatchService) Get(watchName string) (watchResp *utils.WatchParams, err error)

We hope for your understanding.

josh-barker-coles · 2021-02-08T00:02:39Z

Hi @yahavi

Thanks for letting me know.
However, I think its worth discussing the rational behind returning the HTTP response.

Although an error indicates that something has gone wrong, it limits the ability of consumers to handle error conditions.
For example, a Internal server error (500), a Not Found (404) error, and an Unauthorized (401) error are all quite different and may require different actions.

The implication of removing the HTTP response will force consumers to write their own code to parse the string and attempt to handle the generic error.

Additionally, this design decision has wider implications for the artifactory terraform provider https://github.com/jfrog/terraform-provider-artifactory as the future goal is to refactor it to use this client library.
Yet, the 2 clients have different designs in that the old client (www.github.com/atlassian/go-artifactory) returns HTTP Responses, yet the new (jfrog-client-go) does not.

One example is https://github.com/jfrog/terraform-provider-artifactory/blob/master/pkg/artifactory/resource_artifactory_virtual_repository.go#L151-L157

Looking forward to your responses

/cc: @eyalbe4 @DarthFennec @chb0github

eyalbe4 · 2021-02-08T00:45:38Z

Thank you for sharing this input @josh-barker-coles.
We may be able to support this requirement by attaching the HTTP response to the error returned by the APIs. We can build this mechanism in a way that it will seamlessly serve all APIs, existing and future ones.
This apparoach may require a some more thought, but I think this is doable.
Please let me know what you think.

josh-barker-coles · 2021-02-08T22:15:47Z

Hi @eyalbe4, no worries.
Yep, I think that would be great. Do you have an implementation in mind?

It might be good to discuss with the maintainers of https://github.com/jfrog/terraform-provider-artifactory to see how that could work.

Cheers

eyalbe4 requested changes Oct 16, 2020

View reviewed changes

eyalbe4 requested changes Oct 17, 2020

View reviewed changes

josh-barker-coles requested a review from eyalbe4 October 26, 2020 23:53

This was referenced Oct 27, 2020

[Feat] Xray Watch Support jfrog/terraform-provider-artifactory#6

Closed

feat: create xray watch resource jfrog/terraform-provider-artifactory#10

Closed

eyalbe4 approved these changes Nov 6, 2020

View reviewed changes

eyalbe4 mentioned this pull request Nov 6, 2020

The Xray APIs should not use ArtifactoryClientBuilder #224

Closed

josh-barker-coles and others added 15 commits November 9, 2020 10:30

feat(xray): setup service manager for xray endpoint

7809b5a

feat(xray): support xray watch apis

0e6132d

feat(xray): support update on xray watch

d4a29b7

test(xray): refactor tests to separate functions

09fd8fd

refactor(xray): tidy up names and remove redundant methods

69afc2e

refactor(xray): move watch to xray package

1cd9175

test(xray): add more update test cases

8ecf4f7

docs(xray): update readme

070f421

chore(xray): fix golint issues

d1d2252

refactor(xray): move structs and functions to utils file

4ef0b47

test(xray): add tests cases for repository and built types

75a9cbc

docs(xray): document structs, funcs and update readme for watch

ccb28a7

chore(*): ignore go coverage file

1139426

test(xray): fix unit tests

df5fdbe

test(xray): only test xray when an xray url is set

8969dab

josh-barker-coles added 15 commits November 9, 2020 10:31

fix(xray): check error when creating body for update

0a61652

refactory(xray): rename StringFilters to Filters

8f319d8

chore(xray): reorder structs into exported and unexported

c17b085

chore(xray): update comments in code

0a615d2

test(xray): rename test functions and throw response errors separately

14efc5a

fix(xray): use checkError to wrap errors from different sources

128e9d0

chore(xray): document that bundles are not supported and remove place…

38a9f41

…holder function

chore(xray): tidy up logging for watch functions

dee3850

fix(xray): ensure watch url is in the correct format

908ea61

refactor(xray): remove Xray prefix on structs and functions

366943b

refactor(xray): return http responses for watch functions

9d0aa86

fix(xray): fix bug where multiple properties were not set

226ebc3

fix(xray): create default parameters for watch

f141060

fix(xray): add xray details on watch service

1921286

docs(xray): update examples in readme for watch functions

bc2aa2f

josh-barker-coles force-pushed the feat/xray-watch branch from 2333d60 to bc2aa2f Compare November 8, 2020 23:39

eyalbe4 merged commit 5bbc84f into jfrog:dev Nov 9, 2020

Feature: xray watch #211

Feature: xray watch #211

Conversation

josh-barker-coles commented Oct 14, 2020

Description:

Related Issues

github-actions bot commented Oct 14, 2020 • edited Loading

josh-barker-coles commented Oct 14, 2020

josh-barker-coles commented Oct 14, 2020

josh-barker-coles commented Oct 15, 2020

eyalbe4 commented Oct 16, 2020

eyalbe4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalbe4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh-barker-coles commented Oct 19, 2020

josh-barker-coles commented Oct 22, 2020

josh-barker-coles commented Nov 3, 2020

eyalbe4 left a comment

Choose a reason for hiding this comment

josh-barker-coles commented Nov 8, 2020

yahavi commented Feb 7, 2021

josh-barker-coles commented Feb 8, 2021

eyalbe4 commented Feb 8, 2021

josh-barker-coles commented Feb 8, 2021

github-actions bot commented Oct 14, 2020 •

edited

Loading