fix: nodeselector and tolerations for managed services, and managed#287
Conversation
resource secrets are now stored in our database as `readOnly` secrets
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Enhancement
PR Summary: This pull request introduces enhancements to the management of node selectors and tolerations for app and managed services, alongside changes to how managed resource secrets are stored in the database. Specifically, it adds support for specifying node selectors and tolerations directly within the managed service specifications. Additionally, it modifies the handling of managed resource secrets to store them as readOnly secrets in the database, enhancing security and management of sensitive information.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
- Big diff: the diff is too large to approve with confidence.
General suggestions:
- Ensure that the new functionality introduced for node selectors and tolerations is thoroughly documented, including examples of how to specify these in managed service specifications. Clear documentation will aid users in leveraging these new features effectively.
- Consider the impact of storing secrets as
readOnlyin the database on existing workflows and integrations. It may be beneficial to communicate these changes to users who rely on managed resource secrets to ensure a smooth transition. - Review the handling of
readOnlysecrets to ensure that they are correctly marked and managed throughout their lifecycle. This includes verifying that secrets are not inadvertently modified or deleted, preserving their integrity and confidentiality.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| common.ResourceMetadata `json:",inline"` | ||
| SyncStatus t.SyncStatus `json:"syncStatus" graphql:"noinput"` | ||
|
|
||
| IsReadOnly bool `json:"isReadyOnly" graphql:"noinput"` |
There was a problem hiding this comment.
issue (llm): It appears there's a typo in the JSON tag for IsReadOnly. It should likely be isReadOnly instead of isReadyOnly. This typo could lead to serialization/deserialization issues when interacting with this field in JSON format.
| d.resourceEventPublisher.PublishResourceEvent(ctx, umres.GetResourceType(), umres.GetName(), PublishUpdate) | ||
| return errors.NewE(err) | ||
|
|
||
| if mres.SyncedOutputSecretRef != nil { |
There was a problem hiding this comment.
suggestion (llm): I noticed the implementation for handling readOnly secrets has been added, but I don't see any unit tests covering this new logic. It's crucial to ensure that secrets are correctly marked as readOnly and stored in the database as expected. Could you add tests to verify that the SyncedOutputSecretRef is processed correctly, including cases where labels might already exist or the secret data needs to be converted from Data to StringData?
| } | ||
|
|
||
| Github__com___kloudlite___operator___apis___crds___v1__ManagedServiceSpec struct { | ||
| NodeSelector func(childComplexity int) int |
There was a problem hiding this comment.
suggestion (llm): It appears that the schema has been updated to include NodeSelector and Tolerations for managed services. However, I don't see any tests verifying these new schema elements. Integration tests should be added to ensure these fields are correctly handled in the GraphQL layer, including proper parsing and error handling for invalid inputs.
| } | ||
|
|
||
| service.Namespace = "trest" | ||
| service.Namespace = "trest" // this is just to make it pass validation |
There was a problem hiding this comment.
question (llm): I noticed that the namespace is hardcoded to "trest" for validation purposes. While this change might not directly relate to testing, it's crucial to ensure that there are tests covering scenarios where different namespaces are used, especially since this change seems to be a workaround for validation. Can we add or ensure there are tests that validate the behavior of managed services across different namespaces?
| common.ResourceMetadata `json:",inline"` | ||
| SyncStatus t.SyncStatus `json:"syncStatus" graphql:"noinput"` | ||
|
|
||
| IsReadOnly bool `json:"isReadyOnly" graphql:"noinput"` |
There was a problem hiding this comment.
issue (llm): It appears there's a typo in the json tag for IsReadOnly (marked as isReadyOnly). While this is a minor issue, it could lead to unexpected behavior if the field is serialized/deserialized in tests or production code. Please correct this typo. Additionally, ensure there are tests that verify the serialization and deserialization of the Secret struct, particularly focusing on the IsReadOnly field to confirm it behaves as expected.
| } | ||
|
|
||
| type GithubComKloudliteOperatorApisCrdsV1ManagedServiceSpec struct { | ||
| NodeSelector map[string]interface{} `json:"nodeSelector,omitempty"` |
There was a problem hiding this comment.
suggestion (llm): The addition of NodeSelector and Tolerations to the ManagedServiceSpec is a significant update. To ensure these new features work as expected, it's important to have unit tests covering various scenarios, including but not limited to: setting no nodeSelector or tolerations, setting valid nodeSelectors and tolerations, and edge cases where invalid configurations are provided. These tests should verify that the managed services are correctly utilizing these configurations.
| const ( | ||
| ClusterManagedServiceSpec = "spec" | ||
| ClusterManagedServiceSpecMsvcSpec = "spec.msvcSpec" | ||
| ClusterManagedServiceSpecMsvcSpecNodeSelector = "spec.msvcSpec.nodeSelector" |
There was a problem hiding this comment.
suggestion (llm): Given the introduction of constants for NodeSelector and Tolerations, it's crucial to ensure that there are integration tests verifying that these fields are correctly processed and applied within the system. These tests should cover scenarios where these fields are used in API requests, ensuring that the system behaves as expected and that these fields are correctly mapped and utilized in managed services.
fix: nodeselector and tolerations for managed services, and managed
1 participant
Updates
readOnlysecrets