-
Notifications
You must be signed in to change notification settings - Fork 1
chore: transformations cli client #335
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: main
Are you sure you want to change the base?
Conversation
🔒 Scanned for secrets using gitleaks 8.28.0
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
==========================================
+ Coverage 44.03% 44.40% +0.36%
==========================================
Files 192 194 +2
Lines 13908 14103 +195
==========================================
+ Hits 6125 6262 +137
- Misses 7211 7243 +32
- Partials 572 598 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🔒 Scanned for secrets using gitleaks 8.28.0
| if !publishResp.Published { | ||
| return fmt.Errorf("batch publish failed: published=false") | ||
| } |
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.
Do we also provide any reason why the publish failed ?
For logging and auditing purposes in the CLI, we would require it right ?
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.
upon any kind of failure, api throws error. There would be no case where published returns false. Errors get caught in client request (resp, err := client.Do) itself. Added this for consistency. Can remove entire response unmarshalling block
api/client/transformations/types.go
Outdated
| // CreateTransformationRequest is the request body for creating/updating transformations | ||
| type CreateTransformationRequest struct { | ||
| Name string `json:"name"` | ||
| Description string `json:"description,omitempty"` |
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.
Let's handle description as *string here in the request. Setting a description on transformation is a valid request in my opinion right. With it being mentioned as omitempty, we might miss the subtle flow where we will not be able to set it to empty right ?
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.
Done
🔒 Scanned for secrets using gitleaks 8.28.0
🔒 Scanned for secrets using gitleaks 8.28.0
🔒 Scanned for secrets using gitleaks 8.28.0
🔒 Scanned for secrets using gitleaks 8.28.0
fixes DAW-2745