Skip to content
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

feat(bedrock): adopt to cdk best practices #770

Closed
wants to merge 9 commits into from

Conversation

maxtybar
Copy link
Contributor

Fixes #731 and #747


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

Removed `readonly` from actionGroups and knowledgeBases to prevent jsii errors

Signed-off-by: Maksim Tybar <61300968+maxtybar@users.noreply.github.com>
Modifed `addActionGroup` function

Signed-off-by: Maksim Tybar <61300968+maxtybar@users.noreply.github.com>
/**
* Instance of Agent
*/
readonly agentInstance: bedrock.CfnAgent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't include this in the interface, as this implies that to import an existing agent you would need to pass a CfnAgent. I think it would be better to define it only on the Agent Class as a private readonly (internal) attribute (example on CDK lib).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aws-rafams If you have available cycles please take a look at this. There is also issues with IKnowledgeBase on the build. Thanks!

@krokoko
Copy link
Collaborator

krokoko commented Nov 7, 2024

We can also update the KB associateToAgent method to take an IAgent, allowing to associate an imported agent

} else {
this.role = new iam.Role(this, 'Role', {
assumedBy: new iam.ServicePrincipal('bedrock.amazonaws.com'),
roleName: generatePhysicalNameV2(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to use the new capability of generatePhysicalNameV2 here to avoid collisions with name generation

@krokoko
Copy link
Collaborator

krokoko commented Nov 20, 2024

Closing this PR as we will start a new one with the agent refactoring

@krokoko krokoko closed this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(bedrock): agent module refactoring to follow best practices from CDK
4 participants