feat: create xray watch resource #10
Conversation
|
Hi @peters95, thanks for your feedback. I would also expect that the resource for an Xray Policy would be in a separate PR. |
|
Hi @chb0github , will you be taking over this PR too? |
|
@chb0github please review this PR. |
| } | ||
| details.SetUrl(url) | ||
|
|
||
| if username != "" && password != "" { |
There was a problem hiding this comment.
These are not nested cases (so, no else if needed).
Also, please have independent cases for element missing; not all bunched up into 1 error
There was a problem hiding this comment.
Hi @chb0github ,
There are 3 ways someone could authenticate to xray.
- User name and password
- API Key
- Access token
The cases are mutually exclusive.
Hence, the else if.
pkg/artifactory/provider.go
Outdated
| accessToken := d.Get("access_token").(string) | ||
|
|
||
| url := d.Get("xray_url").(string) | ||
| if url[len(url)-1] != '/' { |
There was a problem hiding this comment.
So, a url of gopher://thing will work? Please validate it's a legit url. And, you're not checking that even it's not empty
There was a problem hiding this comment.
I've added some validation on the url.
| client := m.(*ArtClient).XrayClient | ||
|
|
||
| if client == nil { | ||
| return fmt.Errorf("you must specify the xray_url in the provider to manage a watch") |
There was a problem hiding this comment.
But, that's not what you're actually checking for - you're checking for nil, but assuming the reason for a nil is because the url was missing. Asserting client isn't nil is fine, I guess. But the error message has to reflect the issue
There was a problem hiding this comment.
I've updated the error message.
| client := m.(*ArtClient).XrayClient | ||
|
|
||
| if client == nil { | ||
| return fmt.Errorf("you must specify the xray_url in the provider to manage a watch") |
There was a problem hiding this comment.
But, that's not what you're actually checking for - you're checking for nil, but assuming the reason for a nil is because the url was missing. Asserting client isn't nil is fine, I guess. But the error message has to reflect the issue
There was a problem hiding this comment.
I've updated the error message.
| client := m.(*ArtClient).XrayClient | ||
|
|
||
| if client == nil { | ||
| return false, fmt.Errorf("you must specify the xray_url in the provider to manage a watch") |
There was a problem hiding this comment.
But, that's not what you're actually checking for - you're checking for nil, but assuming the reason for a nil is because the url was missing. Asserting client isn't nil is fine, I guess. But the error message has to reflect the issue
There was a problem hiding this comment.
I've updated the error message.
| return false, fmt.Errorf("you must specify the xray_url in the provider to manage a watch") | ||
| } | ||
|
|
||
| watchName := d.Id() |
There was a problem hiding this comment.
Why create this extra variable?
There was a problem hiding this comment.
I've removed the variable
| sort.Strings(packageTypes) | ||
| paths := castToStringArr(repository["paths"].(*schema.Set).List()) | ||
| sort.Strings(paths) | ||
| mimeTypes := castToStringArr(repository["mime_types"].(*schema.Set).List()) |
There was a problem hiding this comment.
Why are you sorting these?
And, please put spaces between the string slices and the sort pair
There was a problem hiding this comment.
I'm sorting the lists to ensure that terraform does not think there is a change if the returned list is ordered differently.
josh-barker-coles
force-pushed
the
feat/xray-watch
branch
from
5a299c2 to
2ca8b27
Compare
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ |
|
Based upon the internal discussions this effort has been put on hold while we align with the product team to understand how we want to implement xray. Given the name of this provider is specific to artifactory we will need to take a step back and evaluate what makes sense from a product roadmap decision before moving forward with this work. In addition, we will need to use the new jfrog cli go client to implement this for both policies and watches when we do pick this work back up. Until then this will be put on hold for now. Apologies for any inconvenience this causes. |
Hi,
This PR implements the
artifactory_watchresource.Note: It only supports the configuration of repositories and builds.
References
Dependencies: