Skip to content

Conversation

@daquinteroflex
Copy link
Collaborator

@daquinteroflex daquinteroflex commented Nov 5, 2025

Greptile Overview

Greptile Summary

This PR introduces a new deployment workflow architecture for TestPyPI and AWS CodeArtifact testing/deployment:

Major Changes:

  • Created new reusable deployment workflow (tidy3d-python-client-deploy.yml) with support for TestPyPI, PyPI (stubbed), and AWS CodeArtifact
  • Refactored release workflow to call the new deployment workflow with configurable targets
  • Updated release type options from draft/final to draft/codeartifact/testpypi/pypi
  • Added AWS CLI to dev Docker container for CodeArtifact support
  • Removed automatic push triggers from docs sync workflow (now workflow_call only)

Critical Issues:

  • Environment variable reference bug in AWS deployment will cause failures (line 264 of deploy workflow)
  • Production PyPI deployment is commented out with only a placeholder echo statement

Confidence Score: 2/5

  • This PR has critical bugs that will cause AWS deployments to fail and PyPI deployments are non-functional
  • Score reflects two critical issues: (1) AWS CodeArtifact deployment has an environment variable reference bug that will cause runtime failures, and (2) production PyPI deployment is stubbed out with a placeholder. While the overall architecture is sound and TestPyPI deployment should work, these blocking issues prevent successful deployment to key targets.
  • .github/workflows/tidy3d-python-client-deploy.yml requires immediate fixes for AWS env variable reference and PyPI placeholder implementation

Important Files Changed

File Analysis

Filename Score Overview
.github/workflows/tidy3d-python-client-deploy.yml 2/5 New deployment workflow with critical issues: env variable reference bug in AWS deployment, production PyPI job stubbed out with placeholder
.github/workflows/tidy3d-python-client-release.yml 4/5 Updated release workflow to call new deploy workflow, added deployment target controls (testpypi/pypi/codeartifact), refactored terminology from 'final' to 'pypi'
.github/workflows/tidy3d-docs-sync-readthedocs-repo.yml 3/5 Removed automatic push triggers for docs sync - workflow now only callable, verify this aligns with intended docs deployment strategy

Sequence Diagram

sequenceDiagram
    participant User
    participant Release as Release Workflow
    participant Deploy as Deploy Workflow
    participant TestPyPI
    participant PyPI
    participant AWS as AWS CodeArtifact
    participant ReadTheDocs

    User->>Release: Trigger with release_tag & release_type
    Release->>Release: Validate tag format
    Release->>Release: Determine workflow scope
    alt Tests Enabled
        Release->>Release: Run client/cli/submodule tests
    end
    
    alt Deploy Enabled
        Release->>Deploy: Call deploy workflow
        Deploy->>Deploy: Validate inputs (at least 1 target)
        Deploy->>Deploy: Build package with Poetry
        Deploy->>Deploy: Upload artifacts
        
        alt deploy_testpypi=true
            Deploy->>TestPyPI: Publish with twine
        end
        
        alt deploy_pypi=true
            Deploy->>Deploy: Wait for TestPyPI (if selected)
            Deploy->>PyPI: Publish (CURRENTLY STUBBED)
        end
        
        alt deploy_aws=true
            Deploy->>AWS: Configure credentials
            Deploy->>AWS: Get auth token
            Deploy->>AWS: Configure package settings
            Deploy->>AWS: Publish with twine
        end
        
        Deploy->>Deploy: Generate deployment summary
        Deploy-->>Release: Return results
    end
    
    alt sync_readthedocs=true
        Release->>ReadTheDocs: Sync docs (workflow_call)
    end
    
    alt sync_branches=true (PyPI releases)
        Release->>Release: Sync main/develop branches
    end
    
    Release-->>User: Workflow complete
Loading

@daquinteroflex daquinteroflex marked this pull request as ready for review November 11, 2025 12:37
@daquinteroflex daquinteroflex merged commit cce6b7b into FXC-3931-update-tidy-3-d-python-client-release-actions-to-be-manually-triggered-and-create-a-tag Nov 11, 2025
@daquinteroflex daquinteroflex deleted the FXC-4001 branch November 11, 2025 12:37
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (1)

  1. .github/workflows/tidy3d-docs-sync-readthedocs-repo.yml, line 35-43 (link)

    style: check that docs sync behavior is correct - automatic push triggers removed, workflow now only callable

5 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile


- name: publish-to-codeartifact
env:
TWINE_PASSWORD: ${{ env.CODEARTIFACT_AUTH_TOKEN }}
Copy link

Choose a reason for hiding this comment

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

logic: CODEARTIFACT_AUTH_TOKEN not available in this env context

Suggested change
TWINE_PASSWORD: ${{ env.CODEARTIFACT_AUTH_TOKEN }}
TWINE_PASSWORD: ${{ env.CODEARTIFACT_AUTH_TOKEN }}
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/tidy3d-python-client-deploy.yml
Line: 264:264

Comment:
**logic:** `CODEARTIFACT_AUTH_TOKEN` not available in this env context

```suggestion
          TWINE_PASSWORD: ${{ env.CODEARTIFACT_AUTH_TOKEN }}
```

How can I resolve this? If you propose a fix, please make it concise.

(needs.deploy-testpypi.result == 'success' || needs.deploy-testpypi.result == 'skipped')
runs-on: ubuntu-latest
steps:
- run: echo "hi"
Copy link

Choose a reason for hiding this comment

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

logic: production PyPI deployment commented out but job still runs

Remove placeholder or implement proper deployment

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/tidy3d-python-client-deploy.yml
Line: 177:177

Comment:
**logic:** production PyPI deployment commented out but job still runs

Remove placeholder or implement proper deployment

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +13 to +14
type: boolean
default: false
Copy link

Choose a reason for hiding this comment

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

style: all deployment targets default to false, requiring manual selection each time

Consider defaulting deploy_testpypi to true for safer testing workflow

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/tidy3d-python-client-deploy.yml
Line: 13:14

Comment:
**style:** all deployment targets default to `false`, requiring manual selection each time

Consider defaulting `deploy_testpypi` to `true` for safer testing workflow

How can I resolve this? If you propose a fix, please make it concise.

xsel \
xclip

RUN apt-get update && apt-get install -y zip unzip curl \
Copy link

Choose a reason for hiding this comment

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

style: curl installed twice (line 7 and line 13) - remove duplicate

Prompt To Fix With AI
This is a comment left during a code review.
Path: dev.Dockerfile
Line: 13:13

Comment:
**style:** `curl` installed twice (line 7 and line 13) - remove duplicate

How can I resolve this? If you propose a fix, please make it concise.

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