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

Add support for requesting Curation waivers to the CLI #280

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

igorz-jf
Copy link
Contributor

@igorz-jf igorz-jf commented Jan 6, 2025

  • The pull request is targeting the dev branch.
  • The code has been validated to compile successfully by running go vet ./....
  • The code has been formatted properly using go fmt ./....
  • All static analysis checks passed.
  • All tests have passed. If this feature is not already covered by the tests, new tests have been added.
  • Updated the Contributing page / ReadMe page / CI Workflow files if needed.
  • All changes are detailed at the description. if not already covered at JFrog Documentation, new documentation have been added.

Adds support for requesting waivers from the CLI. The flow augments the curation audit command, and is effective only for interactive shells.

Copy link
Contributor

@attiasas attiasas left a comment

Choose a reason for hiding this comment

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

LGTM, please:

  1. expend the description in the PR, what is that waiver? can you attach some ScreenShot or snippet of the changes that the user will be faced.
  2. Make sure your PM went over the Text and approve it.
  3. Please change the documentation if needed

}

func (ca *CurationAuditCommand) sendWaiverRequests(pkgs []*PackageStatus, msg string, serverDetails *config.ServerDetails) (requestStatuses []WaiverResponse, err error) {
fmt.Print("Submitting waiver request...\n\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fmt.Print("Submitting waiver request...\n\n")
log.Info("Submitting waiver request...\n\n")

use the log from "github.com/jfrog/jfrog-client-go/utils/log" (you can also use log.Output)

Comment on lines +544 to +547
_, body, _, err := rtManager.Client().SendGet(pkg.BlockedPackageUrl, true, &clientDetails)
if err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_, body, _, err := rtManager.Client().SendGet(pkg.BlockedPackageUrl, true, &clientDetails)
if err != nil {
return nil, err
}
platformResponse, body, _, err := rtManager.Client().SendGet(pkg.BlockedPackageUrl, true, &clientDetails)
if err != nil {
return nil, errors.New("failed while attempting to get waiver:\n" + err.Error())
}
if err = errorutils.CheckResponseStatusWithBody(platformResponse, body, http.StatusOK); err != nil {
return nil, errors.New("got unexpected server response while attempting to get waiver:\n" + err.Error())
}

I would also validate the response status.
Please also wrap error with text to help debugging

Comment on lines +544 to +577
_, body, _, err := rtManager.Client().SendGet(pkg.BlockedPackageUrl, true, &clientDetails)
if err != nil {
return nil, err
}
var resp struct {
Errors []struct {
Status int `json:"status"`
Message string `json:"message"`
} `json:"errors"`
}
if err := json.Unmarshal(body, &resp); err != nil {
return nil, errors.New("failed decoding waiver request status")
}

var id, status, message string
parts := strings.Split(resp.Errors[0].Message, "|")
if len(parts) != 2 {
return nil, errors.New("failed decoding waiver request status")
}
id, status = parts[0], parts[1]
switch status {
case "pending":
message = WaiverRequestPending
case "approved":
message = WaiverRequestApproved
case "forbidden":
message = WaiverRequestForbidden
}
requestStatuses = append(requestStatuses, WaiverResponse{
PkgName: pkg.PackageName,
Status: status,
WaiverID: id,
Explanation: message,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider refactoring this into its own method for readability

Comment on lines +554 to +556
if err := json.Unmarshal(body, &resp); err != nil {
return nil, errors.New("failed decoding waiver request status")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if err := json.Unmarshal(body, &resp); err != nil {
return nil, errors.New("failed decoding waiver request status")
}
if err := json.Unmarshal(body, &resp); err != nil {
return nil, fmt.Errorf("failed decoding waiver request status: " + err.Error())
}

don't ignore the actual error

var id, status, message string
parts := strings.Split(resp.Errors[0].Message, "|")
if len(parts) != 2 {
return nil, errors.New("failed decoding waiver request status")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil, errors.New("failed decoding waiver request status")
return nil, fmt.Errorf("failed decoding waiver request status, got unexpected Error msg format while trying to split into parts (%d expected 2)", len(parts))

describe the error so it will be unique

if len(requestMsg) >= 5 && len(requestMsg) <= 300 {
break
}
fmt.Print("The reason must be between 5 and 300 characters.\n\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fmt.Print("The reason must be between 5 and 300 characters.\n\n")
log.Output("The reason must be between 5 and 300 characters.\n\n")

}

var id, status, message string
parts := strings.Split(resp.Errors[0].Message, "|")
Copy link
Contributor

Choose a reason for hiding this comment

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

why resp.Errors[0] ? what about the other errors? why only one (and is it always exists? could it be empty?)

@@ -461,6 +484,136 @@ func (ca *CurationAuditCommand) auditTree(tech techutils.Technology, results map
return err
}

func getSelectedPackages(requestedRows string, blockedPackages []*PackageStatus) (selectedPackages []*PackageStatus, ok bool) {
validFormat := regexp.MustCompile(`^(all|(\d+(-\d+)?)(,\d+(-\d+)?)*$)`)
Copy link
Contributor

Choose a reason for hiding this comment

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

comment what is the valid format that the regex describes to help readability

func getSelectedPackages(requestedRows string, blockedPackages []*PackageStatus) (selectedPackages []*PackageStatus, ok bool) {
validFormat := regexp.MustCompile(`^(all|(\d+(-\d+)?)(,\d+(-\d+)?)*$)`)
if !validFormat.MatchString(requestedRows) {
fmt.Print("Invalid request format.\n\n")
Copy link
Contributor

@attiasas attiasas Jan 13, 2025

Choose a reason for hiding this comment

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

Suggested change
fmt.Print("Invalid request format.\n\n")
log.Output("Invalid request format.\n\n")

use log.Output, also maybe hint the user what is the valid format if it can be controlled (or add debug log if it coming from our platform for debugging when issue occur)

Comment on lines +499 to +518
parts := strings.Split(requestedRows, ",")
for _, part := range parts {
if strings.Contains(part, "-") {
rangeParts := strings.Split(part, "-")
start, _ := strconv.Atoi(rangeParts[0])
end, _ := strconv.Atoi(rangeParts[1])
for i := start; i <= end; i++ {
if slices.Contains(indices, i) {
continue
}
indices = append(indices, i)
}
} else {
index, err := strconv.Atoi(part)
if err != nil || slices.Contains(indices, index) {
continue
}
indices = append(indices, index)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
parts := strings.Split(requestedRows, ",")
for _, part := range parts {
if strings.Contains(part, "-") {
rangeParts := strings.Split(part, "-")
start, _ := strconv.Atoi(rangeParts[0])
end, _ := strconv.Atoi(rangeParts[1])
for i := start; i <= end; i++ {
if slices.Contains(indices, i) {
continue
}
indices = append(indices, i)
}
} else {
index, err := strconv.Atoi(part)
if err != nil || slices.Contains(indices, index) {
continue
}
indices = append(indices, index)
}
}
rows := strings.Split(requestedRows, ",")
for _, rowStr := range parts {
if strings.Contains(rowStr, "-") {
// Range of rows requested
rangeParts := strings.Split(rowStr, "-")
startRow, _ := strconv.Atoi(rangeParts[0])
endRow, _ := strconv.Atoi(rangeParts[1])
for i := start; i <= end; i++ {
if slices.Contains(indices, i) {
continue
}
indices = append(indices, i)
}
} else {
// Single row requested
index, err := strconv.Atoi(rowStr)
if err != nil || slices.Contains(indices, index) {
continue
}
indices = append(indices, index)
}
}

rename for readability, maybe also refactor this to a separated method
add some comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants