-
Notifications
You must be signed in to change notification settings - Fork 54
feat: Fully operational Amazon Q module v2.0.0 #362
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
Is this ready for review? I see its still marked as a draft PR @harleylrn |
@bpmct Will be ready Monday 😄 |
@@ -0,0 +1,144 @@ | |||
run "required_variables" { |
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 is good, I think it is probably a better approach to testing on this code to use the native Terraform testing framework for unit tests.
2 Items for this
-
As this is new for the Coder team, can you put some light documentation in the README about how to contribute and execute the tests. Link to the official documentation predominantly and feed free to steal some of the language from our ADRs
-
For these tests, can you pin versions, etc to key versions, such as AgentAPI and Q - We want to certify this module with a specific configuration of AgentAPI and Q and know what breaks when things change from other contributors.
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.
- The terraform tests are fully supported by the coder team, it is mentioned in the contribution guide. I don't think anything need to be added there
- Yes, agree, will add this.
type = string | ||
description = "Custom script to run after installing Amazon Q." | ||
default = null | ||
variable "trust_all_tools" { |
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 is inherently dangerous, and we should put some guidance around its use here given that we are saying this is suitable for production like use cases.
2 Options here
-
We opt for Safety and remove this functionality and drive the users towards allow listing the configuration in the configuration file which has some sane, safe configuration they can compliment.
-
We add a warning in the variable description such as "WARNING: Enabling this removes all restrictions on the Amazon Q to perform actions on the launched Workspace Environment, Available CLIs and Configured MCP Services and to local and remote services. DO NOT USE THIS OUTSIDE OF TEST ENVIRONMENTS.
Either way, the default should be false. As we need to put some guidance on tool trust in the readme.
The more i think about this and the danger it exposes, the more i think we should opt for removal - Happy to discuss it with you in more detail internally.
@@ -116,204 +105,106 @@ variable "system_prompt" { | |||
EOT | |||
} | |||
|
|||
variable "ai_prompt" { | |||
variable "auth_tarball" { |
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.
Lets transition this to a new var name called var.auth_bundle or var.auth_key as it is a bit cleaner, and in the fortunate event we get something else in this space such as an API key we can re-use this var without breaking the module interface and having to bump a full version as per semver.
type = string | ||
description = "The initial task prompt to send to Amazon Q." | ||
default = "Please help me with my coding tasks. I'll provide specific instructions as needed." | ||
description = "Base64 encoded, zstd compressed tarball of a pre-authenticated ~/.local/share/amazon-q directory. After running `q login` on another machine, you may generate it with: `cd ~/.local/share/amazon-q && tar -c . | zstd | base64 -w 0`" |
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.
Lets shorten this to description and pass the creation of the content into the README.
} | ||
|
||
# Expose auth tarball as environment variable for install script | ||
resource "coder_env" "auth_tarball" { |
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.
See comment above about the transition of this to another var name.
- Enhanced module configuration and functionality - Improved variable handling and resource management - Updated for better compatibility and performance
install_script = file("${path.module}/scripts/install.sh") | ||
start_script = file("${path.module}/scripts/start.sh") | ||
module_dir_name = ".amazonq" | ||
agent_config = var.agent_config == null ? templatefile("${path.module}/templates/agent-config.json.tpl", { |
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 is good, however I think we should provide a bit more guidance on how to use agent_config, ie, do we pass in an explicit config block, or should we keep the interface consistent and allow the user to pass through the path to an alternate tpl file outside of the module dir based on the environment configuration ?
The documentation will need to be updated to reflect this.
ARCH="$(uname -m)" | ||
case "$ARCH" in | ||
"x86_64") | ||
Q_URL="https://desktop-release.q.us-east-1.amazonaws.com/${ARG_VERSION}/q-x86_64-linux.zip" |
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.
Just thinking about enterprise use cases where direct internet access is unlikely to be allowed, we should consider the introduction of a new var for specifying the Q install URL Prefix.
ie, var.q_install_url with a default value of "https://desktop-release.q.us-east-1.amazonaws.com" so that users can control this. The same naming format, etc that Amazon us for versioning and filenames still apply however.
Given that the URLs for x86 and arm are different, it might be worth passing in a map that covers both of the URLs in one go.
|
||
module "agentapi" { | ||
source = "registry.coder.com/coder/agentapi/coder" | ||
version = "1.1.1" |
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'm not sure if we can pass this version through as a var , however we will need to call it out as a dependancy in the documentation.
ARG_TRUST_ALL_TOOLS='${var.trust_all_tools}' \ | ||
ARG_AI_PROMPT='${base64encode(local.full_prompt)}' \ | ||
ARG_MODULE_DIR_NAME='${local.module_dir_name}' \ | ||
ARG_FOLDER='${var.folder}' \ |
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.
Can we transition away from 'folder' and the var name, I feel it is too vague a descriptor - Something like 'homedir' or similar i feel would be easier to work with.
Feedback on this welcome :)
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.
Hey Matey,
Have dropped some comments through this, let me know if you have any questions.
Appreciate the work, it's shaping up nicely.
Title:
feat: complete amazon-q module v2.0.0 with comprehensive enhancements
Description:
Closes #240
This PR introduces a complete rewrite and enhancement of the amazon-q module, bringing it to version 2.0.0. The module now provides AgentAPI support.
Type of Change
Module Information
Path:
registry/coder/modules/amazon-q
New version:
v2.0.0
Breaking change: [x] Yes [ ] No
Key Features & Enhancements
🚀 Core Functionality
🛠️ Customization & Configuration
📚 Documentation & Testing
d## Breaking Changes
This is a major version update (v2.0.0) with breaking changes:
Testing & Validation
terraform test
- 8/8 tests passing)bun run fmt
)Related Issues
Closes #240 - Amazon Q module enhancement request
Additional Notes
Screenshots: