-
Notifications
You must be signed in to change notification settings - Fork 4
KnowledgeBase Resource #10
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
|
Hi @nesymno. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/assign @knottnt |
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.
Left a few comments.
| Contains information about a knowledge base. | ||
| properties: | ||
| clientToken: |
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.
This field path should be ignored so that it isn't included in the spec. An idempotency token isn't something the user should be setting the in the Spec.
generator.yaml
Outdated
| create: | ||
| name: CreateKnowledgeBase | ||
| read_one: | ||
| name: GetKnowledgeBase | ||
| read_many: | ||
| name: ListKnowledgeBases | ||
| update: | ||
| name: UpdateKnowledgeBase | ||
| delete: | ||
| name: DeleteKnowledgeBase |
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.
Were you seeing the code-generator fail to recognize these operations? I believe it should pick these up automatically.
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.
I had no errors during the code-generation. But I got it, will remove this.
| operation: TagResource | ||
| path: Tags | ||
| exceptions: | ||
| terminal_codes: |
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.
For terminal codes we only want to specify errors where the user definitely must change the resource's Spec in order for the controller to reconcile. Examples, of this would invalid inputs such as the ValidationException. Generally, we don't include ResourceNotFoundException as this may not require a change to resource's Spec. For example the missing resource may be created in the future.
You can find more details here.
| is_required: true | ||
| StorageConfiguration: | ||
| is_required: true | ||
| Tags: |
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.
To fully implement resource tagging I believe you'll need to add some custom hooks similar to those used for the Agent resource.
| properties: | ||
| databaseUser: | ||
| type: string | ||
| type_: |
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.
It looks like the AuthConfiguration.Type field is having a trailing underscore added due to "type" being a Go keyword. In these cases we prefer to force the Go json tag back to the "type". You can see an example of this in the kafka-controller's generator.yaml. It looks like there are a few other instances of type_ in the Spec besides this one.
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.
@knottnt Yeah. What about a case, where field is a slice of structs that contain type_?
I didn't find anything about that in docs and I also checked out a lot of another controllers, but didn't find same case.
That is how I doing that now:
KnowledgeBaseConfiguration.SQLKnowledgeBaseConfiguration.RedshiftConfiguration.StorageConfigurations.Type:
go_tag: json:"type,omitempty"
Where StorageConfigurations = []struct{Type: *string}, but looks like it doesn't work
I saw that I could use type: []*struct{Type: *string} (for example), but how to say to generator that you need to change json tags for each slice element with name 'Type'?
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.
Testing locally it looks like the below works for setting the json tag for the individual elements.
KnowledgeBaseConfiguration.SQLKnowledgeBaseConfiguration.RedshiftConfiguration.StorageConfigurations.Type:
go_tag: json:"type,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.
Okay. I just checked generated yaml and it still contains type_ for the list item.
|
Thanks @nesymno for the contribution! For the E2E tests you can look at the Agent tests for an example. If you have any questions feel free to reach out! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nesymno The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@knottnt I have stuck on e2e tests. Can you check them pls? I know, that tags related stuff will not work now, but crud tests also fails and I got next error: As for me, |
Issue #2117
Description of changes:
TODO:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@knottnt Hey! That is my first contribution to you guys, so can you check it out and give some advices, maybe I did something wrong. At all, I also want to add e2e tests and cover all endpoints from KnowledgeBase resource.