image registry webhook api for image creation and console graphql que…#364
Conversation
…ries exposed for getting, deleting an image and list of images
Reviewer's Guide by SourceryThis pull request implements a new feature for managing registry images in the console application. It adds functionality to create, list, delete, and retrieve registry images, as well as to generate URLs for image webhooks. The changes span across multiple components, including the console app, webhook app, and IAM service. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nxtcoder36 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Avoid hardcoding sensitive information like passwords. (link)
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| return parts[0], "latest" | ||
| } | ||
|
|
||
| func (d *domain) GetRegistryImageURL(ctx ConsoleContext, image string, meta map[string]any) (string, error) { |
There was a problem hiding this comment.
suggestion: Consider returning a structured object instead of a curl command string
Returning a curl command as a string might be limiting and less flexible for clients. Consider returning a structured object with the necessary information (URL, headers, payload) that clients can use to construct their own requests.
func (d *domain) GetRegistryImageURL(ctx ConsoleContext, image string, meta map[string]any) (RegistryImageInfo, error) {
type RegistryImageInfo struct {
URL string
Headers map[string]string
Payload map[string]interface{}
}
| msgTypes "github.com/kloudlite/api/pkg/messaging/types" | ||
| ) | ||
|
|
||
| func processWebhooks(consumer WebhookConsumer, d domain.Domain, logger logging.Logger) error { |
There was a problem hiding this comment.
suggestion: Enhance error handling and consider implementing retry logic
The current error handling in processWebhooks is minimal. Consider implementing more robust error handling, including specific error types and potential retry logic for transient failures. This would improve the reliability of webhook processing.
func processWebhooks(consumer WebhookConsumer, d domain.Domain, logger logging.Logger) error {
maxRetries := 3
backoff := time.Second
return retry.Do(
func() error {
return consumer.Consume(func(msg *msgTypes.ConsumeMsg) error {
logger := logger.WithName("webhook-consumer")
// ... rest of the function
})
},
retry.Attempts(uint(maxRetries)),
retry.Delay(backoff),
retry.DelayType(retry.BackOffDelay),
)
}
| return base64.StdEncoding.EncodeToString([]byte(info)) | ||
| } | ||
|
|
||
| func getImageNameTag(image string) (string, string) { |
There was a problem hiding this comment.
suggestion: Add error handling for malformed image strings
The current implementation assumes a well-formed image string. Consider adding error handling for cases where the image string doesn't contain a colon or has multiple colons. This would make the function more robust against malformed input.
func getImageNameTag(image string) (string, string, error) {
parts := strings.Split(image, ":")
if len(parts) != 2 {
return "", "", fmt.Errorf("invalid image format: %s", image)
}
return parts[0], parts[1], nil
}
| return true, nil | ||
| } | ||
|
|
||
| // CoreDeleteRegistryImage is the resolver for the core_deleteRegistryImage field. |
There was a problem hiding this comment.
issue (complexity): Consider introducing a helper function for context conversion and error handling in resolver functions.
The addition of several new resolver functions for registry image operations has introduced repetitive error handling patterns, increasing overall complexity. To address this, consider introducing a helper function that encapsulates the common context conversion and error handling logic. This refactoring will reduce duplication, improve maintainability, and make the code more concise without sacrificing clarity.
Here's an example of how you could implement this:
func withConsoleContext[T any](ctx context.Context, f func(*entities.ConsoleContext) (T, error)) (T, error) {
cc, err := toConsoleContext(ctx)
if err != nil {
var zero T
return zero, errors.NewE(err)
}
result, err := f(cc)
if err != nil {
return result, errors.NewE(err)
}
return result, nil
}You can then use this helper function to simplify your resolver functions. For example:
func (r *mutationResolver) CoreDeleteRegistryImage(ctx context.Context, image string) (bool, error) {
return withConsoleContext(ctx, func(cc *entities.ConsoleContext) (bool, error) {
if err := r.Domain.DeleteRegistryImage(cc, image); err != nil {
return false, err
}
return true, nil
})
}This approach reduces boilerplate code while maintaining the clear structure of each resolver. It also centralizes error handling, making it easier to modify if needed in the future. Apply this pattern to other similar resolver functions to significantly reduce code duplication and improve overall maintainability.
| return ret | ||
| } | ||
|
|
||
| func (ec *executionContext) marshalNGithub__com___kloudlite___operator___apis___crds___v1__AppContainer2ᚖgithubᚗcomᚋkloudliteᚋapiᚋappsᚋconsoleᚋinternalᚋappᚋgraphᚋmodelᚐGithubComKloudliteOperatorApisCrdsV1AppContainer(ctx context.Context, sel ast.SelectionSet, v *model.GithubComKloudliteOperatorApisCrdsV1AppContainer) graphql.Marshaler { |
There was a problem hiding this comment.
issue (complexity): Consider implementing generic marshaling and unmarshaling functions using reflection to reduce code duplication.
The introduction of the RegistryImage feature has led to significant code duplication, particularly in the marshaling and unmarshaling functions. To reduce complexity and improve maintainability, consider implementing a generic marshaling/unmarshaling function using reflection. Here's a proposed approach:
- Create a generic marshal function:
func genericMarshal(ctx context.Context, sel ast.SelectionSet, v interface{}) graphql.Marshaler {
if v == nil {
return graphql.Null
}
val := reflect.ValueOf(v)
if val.Kind() == reflect.Ptr {
if val.IsNil() {
return graphql.Null
}
val = val.Elem()
}
result := make(map[string]interface{})
typ := val.Type()
for i := 0; i < val.NumField(); i++ {
field := typ.Field(i)
value := val.Field(i)
if value.CanInterface() {
result[field.Name] = value.Interface()
}
}
return graphql.MarshalMap(result)
}- Create a generic unmarshal function:
func genericUnmarshal(ctx context.Context, v interface{}, target interface{}) error {
targetValue := reflect.ValueOf(target)
if targetValue.Kind() != reflect.Ptr || targetValue.IsNil() {
return fmt.Errorf("target must be a non-nil pointer")
}
targetValue = targetValue.Elem()
targetType := targetValue.Type()
mapValue, ok := v.(map[string]interface{})
if !ok {
return fmt.Errorf("input must be a map[string]interface{}")
}
for i := 0; i < targetValue.NumField(); i++ {
field := targetType.Field(i)
fieldValue := targetValue.Field(i)
if value, ok := mapValue[field.Name]; ok {
if fieldValue.CanSet() {
convertedValue := reflect.ValueOf(value)
if convertedValue.Type().AssignableTo(fieldValue.Type()) {
fieldValue.Set(convertedValue)
}
}
}
}
return nil
}- Use these generic functions to replace repetitive code. For example:
func (ec *executionContext) marshalORegistryImage2ᚖgithubᚗcomᚋkloudliteᚋapiᚋappsᚋconsoleᚋinternalᚋentitiesᚐRegistryImage(ctx context.Context, sel ast.SelectionSet, v *entities.RegistryImage) graphql.Marshaler {
return genericMarshal(ctx, sel, v)
}
func (ec *executionContext) unmarshalORegistryImageIn2ᚖgithubᚗcomᚋkloudliteᚋapiᚋappsᚋconsoleᚋinternalᚋentitiesᚐRegistryImage(ctx context.Context, v interface{}) (*entities.RegistryImage, error) {
var result entities.RegistryImage
err := genericUnmarshal(ctx, v, &result)
return &result, err
}This approach will significantly reduce code duplication while maintaining type safety. You may need to add some type-specific logic for certain fields, but the bulk of the repetitive code can be replaced with these generic functions. Remember to handle error cases and edge conditions appropriately in the generic functions.
| CreationTime string `json:"creationTime"` | ||
| ID repos.ID `json:"id"` | ||
| MarkedForDeletion *bool `json:"markedForDeletion,omitempty"` | ||
| Password string `json:"password"` |
There was a problem hiding this comment.
🚨 issue (security): Avoid hardcoding sensitive information like passwords.
The Password field in RegistryImageCredentials should not be hardcoded or stored in plain text. Consider using a secure method to handle sensitive information.
2 participants
Summary by Sourcery
Add support for managing registry images through GraphQL API, including creating, deleting, and listing images. Implement a webhook consumer to process registry image webhooks and integrate with NATS Jetstream for message handling. Update the build and deployment configurations to support these new features.
New Features:
Enhancements:
Build:
Deployment:
Tests:
Chores: