Skip to content

replace dynmodb with s3#58

Open
ereli-sevenai wants to merge 7 commits intoduplocloud:mainfrom
OkamiAI:main
Open

replace dynmodb with s3#58
ereli-sevenai wants to merge 7 commits intoduplocloud:mainfrom
OkamiAI:main

Conversation

@ereli-sevenai
Copy link
Copy Markdown

@ereli-sevenai ereli-sevenai commented Jul 22, 2025

User description

This pull request includes changes to streamline setup processes and improve backend configuration for AWS in Terraform. The most important changes involve replacing the pip installation mechanism with uv, enhancing error handling, and modifying AWS backend configuration to use lockfiles instead of DynamoDB tables.

Setup process improvements:

  • setup/action.yml: Replaced the pip upgrade step with the installation of uv using astral-sh/setup-uv@v5.
  • setup/action.yml: Updated the pip install commands to use uv pip install with the --system flag and added error handling (|| exit 1) for robustness.

AWS backend configuration changes:

  • terraform-module/setup.sh: Modified AWS backend configuration to use lockfiles (use_lockfile=true) instead of DynamoDB tables for managing state locks.

PR Type

Enhancement


Description

  • Replace pip with uv for Python package installation

  • Switch AWS backend from DynamoDB to S3 lockfile

  • Add error handling for package installation failures

  • Update backend configuration messaging


Diagram Walkthrough

flowchart LR
  A["pip installation"] --> B["uv installation"]
  C["DynamoDB table locks"] --> D["S3 lockfile locks"]
  E["Basic install"] --> F["Install with error handling"]
Loading

File Walkthrough

Relevant files
Enhancement
action.yml
Migrate from pip to uv package manager                                     

setup/action.yml

  • Replace pip upgrade step with uv installation using
    astral-sh/setup-uv@v5
  • Update duploctl installation to use uv pip install with --system flag
  • Add error handling with || exit 1 for installation failures
+5/-6     
Configuration changes
setup.sh
Switch AWS backend to S3 lockfile                                               

terraform-module/setup.sh

  • Replace DynamoDB table backend config with use_lockfile=true
  • Remove dynamodb_table parameter from backend configuration
  • Update echo message to reflect S3 lockfile usage
+2/-2     

@qodo-code-review
Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Configuration Change

The switch from DynamoDB table locking to S3 lockfile requires validation that the Terraform backend supports this configuration and that concurrent state operations will be properly handled. Verify that use_lockfile=true is a valid backend configuration option for the S3 backend being used.

-backend-config=use_lockfile=true
-backend-config=region=$DUPLO_DEFAULT_REGION
Error Handling

The error handling with || exit 1 may not provide sufficient context about installation failures. Consider adding more descriptive error messages or logging to help with debugging when uv installation fails.

  uv pip install duplocloud-client --system || exit 1
else
  uv pip install duplocloud-client==$VERSION --system || exit 1

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jul 22, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Remove invalid backend configuration parameter

The use_lockfile=true parameter is not a valid Terraform backend configuration
option for S3. S3 backend doesn't support this parameter and will cause
initialization to fail.

terraform-module/setup.sh [13]

--backend-config=use_lockfile=true
+# Remove this line entirely as S3 backend doesn't support use_lockfile parameter
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies that use_lockfile=true is an invalid parameter for the Terraform S3 backend, and leaving it in would cause the terraform init command to fail.

High
  • Update

@ereli-sevenai
Copy link
Copy Markdown
Author

use_lockfile

use_lockfile
image

Comment thread setup/action.yml
shell: bash
run: pip install --upgrade pip

- name: Install uv
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove this for now. If you really want it, make another PR for this and make it optional.

Comment thread setup/action.yml
run: |
if [[ "$VERSION" == "latest" ]]; then
pip install duplocloud-client
uv pip install duplocloud-client --system || exit 1
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

just use pip here.

Comment thread terraform-module/setup.sh
echo "AWS enabled, setting up backend config for S3 with lockfile"
ARGS+=(
-backend-config=dynamodb_table="${PREFIX}-${DUPLO_ACCOUNT_ID}-lock"
-backend-config=use_lockfile=true
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This part I am ok with. This is the important part.

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.

2 participants