Fix/import managed resources#346
Conversation
…pi into fix/import-managed-resources
Reviewer's Guide by SourceryThis pull request introduces the ability to import managed resources and treat root credentials for integrated services as integrated resources. It also allows environments to import and use these integrated resources. 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 @nxtcoder17 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 13 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| } | ||
|
|
||
| // mutations | ||
| func (d *domain) CreateRootManagedResource(ctx ConsoleContext, accountNamespace string, mres *entities.ManagedResource) (*entities.ManagedResource, error) { |
There was a problem hiding this comment.
suggestion: Consider adding validation for the input parameters.
Adding validation for accountNamespace and mres can help catch potential issues early and improve the robustness of the function.
| func (d *domain) CreateRootManagedResource(ctx ConsoleContext, accountNamespace string, mres *entities.ManagedResource) (*entities.ManagedResource, error) { | |
| func (d *domain) CreateRootManagedResource(ctx ConsoleContext, accountNamespace string, mres *entities.ManagedResource) (*entities.ManagedResource, error) { | |
| if accountNamespace == "" { | |
| return nil, errors.New("accountNamespace cannot be empty") | |
| } | |
| if mres == nil { | |
| return nil, errors.New("managed resource cannot be nil") | |
| } | |
| if err := d.canPerformActionInAccount(ctx, iamT.CreateManagedResource); err != nil { | |
| return nil, errors.NewE(err) | |
| } |
| } | ||
|
|
||
| // ManagedResource is the resolver for the ManagedResource field. | ||
| func (r *importedManagedResourceResolver) ManagedResource(ctx context.Context, obj *entities.ImportedManagedResource) (*entities.ManagedResource, error) { |
There was a problem hiding this comment.
suggestion: Consider handling potential nil values for obj.
Adding a nil check for obj can prevent potential runtime panics and improve the robustness of the function.
| func (r *importedManagedResourceResolver) ManagedResource(ctx context.Context, obj *entities.ImportedManagedResource) (*entities.ManagedResource, error) { | |
| func (r *importedManagedResourceResolver) ManagedResource(ctx context.Context, obj *entities.ImportedManagedResource) (*entities.ManagedResource, error) { | |
| if obj == nil { | |
| return nil, errors.New("imported managed resource is nil") | |
| } | |
| cc, err := toConsoleContext(ctx) | |
| if err != nil { |
| return pr, nil | ||
| } | ||
|
|
||
| func fromDataToStringData(secret *corev1.Secret) { |
There was a problem hiding this comment.
suggestion (bug_risk): Consider adding a nil check for secret.
Adding a nil check for secret can prevent potential runtime panics and improve the robustness of the function.
| func fromDataToStringData(secret *corev1.Secret) { | |
| func fromDataToStringData(secret *corev1.Secret) { | |
| if secret == nil { | |
| return | |
| } | |
| if secret.StringData == nil { | |
| secret.StringData = make(map[string]string, len(secret.Data)) | |
| } |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| func (d *domain) ImportManagedResource(ctx ManagedResourceContext, mresName string, importName string) (*entities.ImportedManagedResource, error) { |
There was a problem hiding this comment.
suggestion: Consider adding validation for input parameters.
It would be beneficial to validate mresName and importName to ensure they are not empty or invalid before proceeding with the function logic.
| func (d *domain) ImportManagedResource(ctx ManagedResourceContext, mresName string, importName string) (*entities.ImportedManagedResource, error) { | |
| func (d *domain) ImportManagedResource(ctx ManagedResourceContext, mresName string, importName string) (*entities.ImportedManagedResource, error) { | |
| if mresName == "" || importName == "" { | |
| return nil, fmt.Errorf("mresName and importName must not be empty") | |
| } |
| return imr, nil | ||
| } | ||
|
|
||
| func (d *domain) DeleteImportedManagedResource(ctx ResourceContext, importName string) error { |
There was a problem hiding this comment.
🚨 suggestion (security): Consider adding a confirmation step before deletion.
To prevent accidental deletions, consider adding a confirmation step or a soft delete mechanism.
| func (d *domain) DeleteImportedManagedResource(ctx ResourceContext, importName string) error { | |
| func (d *domain) DeleteImportedManagedResource(ctx ResourceContext, importName string) error { | |
| confirmation := getConfirmationFromUser() | |
| if !confirmation { | |
| return errors.New("deletion aborted by user") | |
| } | |
| if err := d.canMutateResourcesInEnvironment(ctx); err != nil { | |
| return errors.NewE(err) | |
| } |
| return ResourceTypeImportedManagedResource | ||
| } | ||
|
|
||
| type ManagedResourceRef struct { |
There was a problem hiding this comment.
suggestion: Consider embedding common fields.
If ManagedResourceRef is used in multiple places, consider embedding it in other structs to reduce redundancy.
| type ManagedResourceRef struct { | |
| type CommonFields struct { | |
| ID repos.ID `json:"id"` | |
| Name string `json:"name"` | |
| } | |
| type ManagedResourceRef struct { | |
| CommonFields | |
| } |
| @@ -22948,9 +24796,9 @@ func (ec *executionContext) _ImagePullSecretPaginatedRecords_pageInfo(ctx contex | |||
| return ec.marshalNPageInfo2ᚖgithubᚗcomᚋkloudliteᚋapiᚋappsᚋconsoleᚋinternalᚋappᚋgraphᚋmodelᚐPageInfo(ctx, field.Selections, res) | |||
| } | |||
|
|
|||
| func (ec *executionContext) fieldContext_ImagePullSecretPaginatedRecords_pageInfo(ctx context.Context, field graphql.CollectedField) (fc *graphql.FieldContext, err error) { | |||
| func (ec *executionContext) fieldContext_ImportedManagedResourcePaginatedRecords_pageInfo(ctx context.Context, field graphql.CollectedField) (fc *graphql.FieldContext, err error) { | |||
There was a problem hiding this comment.
suggestion: Consider renaming the function for consistency.
The function name fieldContext_ImportedManagedResourcePaginatedRecords_pageInfo is quite long and could be shortened for better readability. Consider renaming it to something more concise while maintaining clarity.
| func (ec *executionContext) fieldContext_ImportedManagedResourcePaginatedRecords_pageInfo(ctx context.Context, field graphql.CollectedField) (fc *graphql.FieldContext, err error) { | |
| func (ec *executionContext) fieldCtx_ImportedManagedResourcePageInfo(ctx context.Context, field graphql.CollectedField) (fc *graphql.FieldContext, err error) { |
| @@ -32768,6 +34608,92 @@ | |||
| return fc, nil | |||
| } | |||
|
|
|||
| func (ec *executionContext) _Query_core_listImportedManagedResources(ctx context.Context, field graphql.CollectedField) (ret graphql.Marshaler) { | |||
There was a problem hiding this comment.
suggestion (performance): Consider adding pagination support.
For the listImportedManagedResources query, consider adding pagination support to handle large datasets efficiently.
| func (ec *executionContext) _Query_core_listImportedManagedResources(ctx context.Context, field graphql.CollectedField) (ret graphql.Marshaler) { | |
| func (ec *executionContext) _Query_core_listImportedManagedResources(ctx context.Context, field graphql.CollectedField, first *int, after *string) (ret graphql.Marshaler) { |
| @@ -38103,6 +40082,113 @@ | |||
| return it, nil | |||
| } | |||
|
|
|||
| func (ec *executionContext) unmarshalInputGithub__com___kloudlite___api___apps___console___internal___entities__ManagedResourceRefIn(ctx context.Context, obj interface{}) (model.GithubComKloudliteAPIAppsConsoleInternalEntitiesManagedResourceRefIn, error) { | |||
There was a problem hiding this comment.
suggestion: Consider breaking down long function names.
The function name unmarshalInputGithub__com___kloudlite___api___apps___console___internal___entities__ManagedResourceRefIn is very long and could be broken down for better readability and maintainability.
| func (ec *executionContext) unmarshalInputGithub__com___kloudlite___api___apps___console___internal___entities__ManagedResourceRefIn(ctx context.Context, obj interface{}) (model.GithubComKloudliteAPIAppsConsoleInternalEntitiesManagedResourceRefIn, error) { | |
| func (ec *executionContext) unmarshalInputManagedResourceRefIn(ctx context.Context, obj interface{}) (model.GithubComKloudliteAPIAppsConsoleInternalEntitiesManagedResourceRefIn, error) { |
| return it, nil | ||
| } | ||
|
|
||
| func (ec *executionContext) unmarshalInputGithub__com___kloudlite___api___pkg___types__SyncStatusIn(ctx context.Context, obj interface{}) (types.SyncStatus, error) { |
There was a problem hiding this comment.
suggestion: Consider adding validation for input fields.
For the unmarshalInputGithub__com___kloudlite___api___pkg___types__SyncStatusIn function, consider adding validation for the input fields to ensure data integrity.
Fix/import managed resources
2 participants
Description
Summary by Sourcery
This pull request introduces the capability to create and manage integrated resources, including root credentials for integrated services. It also allows environments to import and use these resources. The changes include updates to gRPC interfaces, domain logic, and GraphQL schema and resolvers.