Skip to content

Conversation

@ahusseinali
Copy link
Contributor

@ahusseinali ahusseinali commented Oct 23, 2024

Problem

SageMaker IAM are not automatically picked up from node environment for Amazon Q Code Completion. However, this feature is supported in AWS-Toolkit and was working fine for code completion before Amazon Q codebase split from aws-toolkit logic

Solution

  • Bring the IAM credential provider to Amazon Q extension activation
  • The scope is narrowed down to Sagemaker environment for safety and since it's an existing feature, no extra tests need to be added

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

SM IAM permissions have been working as of v1.99 and now it's broken. This change brings it back to support code completion for SM code editor
@ahusseinali ahusseinali requested a review from a team as a code owner October 23, 2024 16:42
@github-actions
Copy link

This pull request modifies code in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@@ -0,0 +1,4 @@
{
"type": "Bug Fix",
"description": "Use SM IAM Credentials for Code Completion"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for including a changelog. Please review the changelog guidelines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it need further refinement? I can take your proposal on that. How much context should be stated in it?

@justinmk3
Copy link
Contributor

justinmk3 commented Oct 23, 2024

The scope is narrowed down to Sagemaker environment for safety and since it's an existing feature,

Please evaluate (based on the usages in the code) whether it makes sense to enable in general. That's part of the work here.

@ahusseinali
Copy link
Contributor Author

Please evaluate (based on the usages in the code) whether it makes sense to enable in general. That's part of the work here.

I initially had it with a wider scope, but I believe it's risky as the logic has implied assumption that the code is loaded in a container environment with IAM permissions passed to that environment. It's hard to reason about whether it should be enabled in general unless we provide Chat support as well for IAM (which is WIP)

@ahusseinali ahusseinali closed this Nov 1, 2024
@ahusseinali
Copy link
Contributor Author

Closing this change as it was merged as part of a different change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants