Skip to content

feat(cli): add secrets list CLI command and github ci secrets masking #520

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

Merged
merged 14 commits into from
Apr 30, 2025

Conversation

aaronsteers
Copy link
Contributor

@aaronsteers aaronsteers commented Apr 29, 2025

New airbyte-cdk secrets list command. Prints the secrets that are labeled with the connector name. It also hyperlinks the name of the secret to the google cloud console URL, where you could review versions, update the secret, or review other updates.

Note: this command does not download the secret itself - it only gets a handle to the secret, metadata only, and no sensitive data is downloaded. (Use airbyte-cdk secrets fetch when you actually need to download the secrets.)

This PR also adds GitHub secrets masking, based on prior logic in the ci_credentials project.


image
image

Mousing over a secret name will show the clickable URL:

image

https://www.loom.com/share/5f04f8ef26634894910a4f38790bd2ff

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Added a command to list secrets for a connector, displaying secret details in a formatted table.
    • Introduced a new CLI option to print GitHub Actions secret masks during secret fetching in CI environments.
  • Improvements
    • Enhanced the fetch command with better error handling, secret file management, and security measures.
    • Added connector name resolution from directory with validation.
  • Bug Fixes
    • Standardized error messages for missing credentials and required packages.
  • Refactor
    • Streamlined secret fetching and writing logic for better maintainability.
  • Documentation
    • Updated CLI help strings for improved clarity.

@github-actions github-actions bot added the enhancement New feature or request label Apr 29, 2025
@aaronsteers aaronsteers marked this pull request as ready for review April 29, 2025 19:16
Copy link
Contributor

coderabbitai bot commented Apr 29, 2025

📝 Walkthrough

Walkthrough

The changes introduce significant refactoring and new features to the secrets management CLI in the Airbyte CDK. The _secrets.py module now includes a new list command for displaying secrets associated with a connector, alongside a refactored fetch command that leverages new helper functions for modularity and error handling. Imports for Google Secret Manager are now conditional, and the code includes improved error messaging and type annotations. Additionally, a new utility function, resolve_connector_name, is added to _util.py for extracting connector names from directory paths with validation. The rich package was added as a new dependency to support formatted CLI output.

Changes

File(s) Change Summary
airbyte_cdk/cli/airbyte_cdk/_secrets.py Refactored the fetch command to use helper functions for client creation, secret fetching, file path resolution, and writing secrets. Added a new list command to display secrets in a table. Introduced multiple new internal helper functions. Improved error handling and modularity.
airbyte_cdk/cli/airbyte_cdk/_util.py Added a new function resolve_connector_name to extract and validate connector names from directory paths, raising errors for invalid or missing directories.
pyproject.toml Added rich = "*" as a new dependency under [tool.poetry.dependencies] to support rich-formatted CLI output.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI
    participant GSMClient as Google Secret Manager Client
    participant FileSystem

    User->>CLI: Run 'secrets fetch' or 'secrets list' command
    CLI->>CLI: Resolve connector name (resolve_connector_name)
    CLI->>CLI: Get GSM client (_get_gsm_secrets_client)
    CLI->>GSMClient: Fetch secrets with label filtering (_fetch_secret_handles)
    alt fetch command
        loop For each secret
            CLI->>CLI: Determine file path (_get_secret_filepath)
            CLI->>FileSystem: Write secret to file (_write_secret_file)
        end
        CLI->>User: Output number of secrets written or error
    else list command
        CLI->>User: Display secrets in table format
    end
Loading

Would you like me to also generate a comparison sequence diagram showing the old vs new control flow for the fetch command, wdyt?


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d23bbb9 and cd86b1d.

📒 Files selected for processing (1)
  • airbyte_cdk/cli/airbyte_cdk/_secrets.py (4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (1)
airbyte_cdk/cli/airbyte_cdk/_util.py (2)
  • resolve_connector_name (46-69)
  • resolve_connector_name_and_directory (8-43)
⏰ Context from checks skipped due to timeout of 90000ms (10)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Analyze (python)
  • GitHub Check: preview_docs
  • GitHub Check: SDM Docker Image Build
🔇 Additional comments (12)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (12)

2-28: Great enhancement to documentation!

The markdown-formatted docstring with fenced code blocks makes the documentation much more readable and user-friendly. The added examples for both fetch and the new list command provide clear guidance for users.


57-63: Good conditional import approach

The try-except pattern for Google Secret Manager imports allows the module to load even when the dependencies aren't installed, with errors deferred until runtime when the functionality is actually used.


92-104: Nice addition of CI secrets masking option

The addition of the --print-ci-secrets-masks flag is a useful feature for CI environments. This will help prevent sensitive information from appearing in logs.


114-120: Clear warning about security implications

Good job adding a clear warning about the security implications of printing secrets to STDOUT and limiting this functionality to CI environments only.


123-173: Well-refactored fetch command implementation

The fetch command has been nicely refactored to use modular helper functions, making the code more maintainable. The security check for CI environment before printing masks is an important safeguard.


250-274: Clean secret handle fetching implementation

The _fetch_secret_handles function properly checks for the availability of the secretmanager package and raises a helpful error message if it's not installed. The filter by connector label is a good approach.


308-314: Good secret directory setup with gitignore

Automatically creating the secrets directory and adding a .gitignore file to prevent secrets from being committed is a good security practice.


353-376: Well-implemented CI secret masking

The function for printing GitHub Actions secret masks is well-implemented with appropriate checks for the CI environment. Good job following GitHub Actions documentation for masking.


378-397: Recursive config masking with type issue

The recursive approach to mask nested secrets is well thought out, but there's a potential type error in line 395.

-            if isinstance(value, (dict, list)):
+            if isinstance(value, (dict, list)):

This is already correct! Previous review comments flagged a similar issue elsewhere using dict | list syntax, but you're already using the correct tuple approach here.


399-412: Good secret property detection

The case-insensitive approach to detect secret properties is a good practice to prevent secrets from leaking. The partial matching approach will catch more variations of secret field names.


414-425: Smart caching of spec mask with lru_cache

Using lru_cache to cache the spec mask is a good optimization. The error handling for the HTTP request and YAML parsing is also well-implemented.


30-56:

✅ Verification successful

Well-structured imports with proper error handling

The addition of typing annotations, logging setup, and conditional imports is a good practice. I see you're using lru_cache for performance optimization on the spec mask fetching too.

One thing to note - previous review comments mentioned issues with dependency declarations. Have you confirmed that requests and yaml are properly declared as direct dependencies in the package setup? wdyt?


🏁 Script executed:

#!/bin/bash
# Check if requests and yaml are declared as dependencies in pyproject.toml
grep -r "requests\|PyYAML\|pyyaml\|yaml" --include="pyproject.toml" --include="setup.py" .

Length of output: 370


Verified direct dependencies

I’ve confirmed that both requests = "*" and PyYAML = "^6.0.1" are listed under [tool.poetry.dependencies] in pyproject.toml. No further changes to dependency declarations are needed here.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
airbyte_cdk/cli/airbyte_cdk/_util.py (1)

46-69: Clean implementation of connector name resolution!

This function properly extracts and validates connector names from directory paths with good error messages. It elegantly complements the existing resolve_connector_name_and_directory function for cases when you already have a directory path.

Just a minor suggestion - would it make sense to add a guard against non-existing directories by checking connector_directory.exists() as well? wdyt?

if not connector_directory:
    raise FileNotFoundError(
        "Connector directory does not exist or cannot be found. Please provide a valid "
        "connector directory."
    )
+if not connector_directory.exists():
+    raise FileNotFoundError(
+        f"Connector directory {connector_directory.absolute()} does not exist. Please provide a valid "
+        "connector directory."
+    )
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a1dd40b and 93b251c.

📒 Files selected for processing (2)
  • airbyte_cdk/cli/airbyte_cdk/_secrets.py (3 hunks)
  • airbyte_cdk/cli/airbyte_cdk/_util.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (1)
airbyte_cdk/cli/airbyte_cdk/_util.py (2)
  • resolve_connector_name (46-69)
  • resolve_connector_name_and_directory (8-43)
🪛 GitHub Actions: Linters
airbyte_cdk/cli/airbyte_cdk/_secrets.py

[error] 191-191: mypy error: Secret? has no attribute "name" (attr-defined)


[error] 192-192: mypy error: Secret? has no attribute "labels" (attr-defined)


[error] 193-193: mypy error: Secret? has no attribute "create_time" (attr-defined)


[error] 230-230: mypy error: Secret? has no attribute "name" (attr-defined)


[error] 272-272: mypy error: Secret? has no attribute "labels" (attr-defined)


[error] 273-273: mypy error: Secret? has no attribute "labels" (attr-defined)

🪛 GitHub Actions: Dependency Analysis
airbyte_cdk/cli/airbyte_cdk/_secrets.py

[error] 28-28: 'rich' imported but it is a transitive dependency (DEP003)


[error] 29-29: 'rich' imported but it is a transitive dependency (DEP003)

⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (6)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (6)

51-57: Type ignore is appropriate here

Good call on using # type: ignore to suppress the type checking issue with the help text.


91-127: Nice refactoring of the fetch command!

This refactoring significantly improves the code organization by breaking down the functionality into smaller, reusable functions. The command flow is now much clearer and easier to understand.

I especially like the improved error reporting when no secrets are found.


199-223: Good approach to fetching secret handles

The type annotation with comment-based # type: ignore is a reasonable workaround, though using cast would be more type-safe as suggested earlier.


225-234: File permission handling is a nice security touch!

Setting the file permissions to 0o600 (owner read/write only) is a good security practice for secret files.

🧰 Tools
🪛 GitHub Actions: Linters

[error] 230-230: mypy error: Secret? has no attribute "name" (attr-defined)


236-264: Thoughtful directory setup with .gitignore

Automatically creating a .gitignore file with '*' in the secrets directory is a good safeguard against accidentally committing secrets.


267-276: Good handling of custom filenames via labels

Using the 'filename' label to customize the output file is a nice touch!

🧰 Tools
🪛 GitHub Actions: Linters

[error] 272-272: mypy error: Secret? has no attribute "labels" (attr-defined)


[error] 273-273: mypy error: Secret? has no attribute "labels" (attr-defined)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (1)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (1)

130-197: ⚠️ Potential issue

Excellent new list command, but type errors need fixing!

The new command provides a valuable way to view available secrets without downloading them. The rich table output looks fantastic.

However, there are several mypy errors related to accessing attributes of Secret objects. Could we fix this with cast operations? wdyt?

for secret in secrets:
    table.add_row(
-        secret.name.split("/secrets/")[-1],  # Name of the secret, without the prefix
-        "\n".join([f"{k}={v}" for k, v in secret.labels.items()]),
-        str(secret.create_time),
+        cast(str, secret.name).split("/secrets/")[-1],  # Name of the secret, without the prefix
+        "\n".join([f"{k}={v}" for k, v in cast(dict, secret.labels).items()]),
+        str(cast(Any, secret.create_time)),
    )
🧰 Tools
🪛 GitHub Actions: Linters

[error] 192-192: mypy: Secret? has no attribute "name" (attr-defined)


[error] 193-193: mypy: Secret? has no attribute "labels" (attr-defined)


[error] 194-194: mypy: Secret? has no attribute "create_time" (attr-defined)

🧹 Nitpick comments (2)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (2)

19-50: Clean import structure with conditional dependencies!

I like the approach of conditionally importing Google Secret Manager modules and providing helpful error messages at runtime. The typing additions are also good for maintainability.

One small suggestion - would it make sense to use the TypeVar to create a more specific type hint for Secret? Perhaps something like:

- Secret: type | None
+ # Define a type alias that can be used when the module isn't loaded
+ SecretType = TypeVar("SecretType", bound="Any")
+ Secret: type[SecretType] | None

This might help with some of the type checking issues we're seeing elsewhere. wdyt?


200-224: Helpful function for fetching secret handles!

This extraction of secret handling logic makes the code more maintainable. The type annotation with the comment # type: ignore is a pragmatic approach to dealing with the conditional import of Secret.

One suggestion - would it be more readable to use a list comprehension directly in the return statement rather than a temporary list? wdyt?

-    secrets = client.list_secrets(
-        request=secretmanager.ListSecretsRequest(
-            parent=parent,
-            filter=filter_string,
-        )
-    )
-    return [s for s in secrets]
+    return list(client.list_secrets(
+        request=secretmanager.ListSecretsRequest(
+            parent=parent,
+            filter=filter_string,
+        )
+    ))
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93b251c and d6d0764.

⛔ Files ignored due to path filters (1)
  • poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • airbyte_cdk/cli/airbyte_cdk/_secrets.py (3 hunks)
  • pyproject.toml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • pyproject.toml
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/cli/airbyte_cdk/_secrets.py

[error] 192-192: mypy: Secret? has no attribute "name" (attr-defined)


[error] 193-193: mypy: Secret? has no attribute "labels" (attr-defined)


[error] 194-194: mypy: Secret? has no attribute "create_time" (attr-defined)


[error] 231-231: mypy: Secret? has no attribute "name" (attr-defined)


[error] 273-273: mypy: Secret? has no attribute "labels" (attr-defined)


[error] 274-274: mypy: Secret? has no attribute "labels" (attr-defined)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: SDM Docker Image Build
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (4)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (4)

92-128: Great modularization of the fetch command!

The refactored implementation is much cleaner with helper functions that improve readability and maintainability. Breaking down the process into smaller units of work follows good software engineering principles.

The error handling for when no secrets are found is also a nice touch for better user experience.


237-266: Nice helper for managing the secrets directory!

I like the approach of ensuring the secrets directory exists and automatically creating a .gitignore file to prevent accidental commits of secrets. This follows security best practices by making it harder to accidentally leak sensitive information.


279-297: Well-structured client initialization!

The function handles both the case when the Google Secret Manager package is not installed and when credentials are not provided. It properly constructs the client from service account info.

A small security note - the error message correctly guides users to set environment variables rather than hardcoding credentials, which is a good security practice.


54-54: Small type ignore comment

The # type: ignore comment is a pragmatic solution to handle type checking issues with the help text. It's focused only on the specific line where needed, which is good practice.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (4)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (4)

1-302: Consider addressing 'rich' dependency warning

Based on previous review comments, there's a warning about 'rich' being a transitive dependency. Would you consider adding it explicitly to your project dependencies?

In pyproject.toml:

rich = "^13.5.3"

Or in setup.py:

install_requires=[
    # …,
    "rich>=13.5.3",
],

wdyt?


127-199: 🛠️ Refactor suggestion

Excellent addition of the list command!

The new command provides a useful way to view available secrets without downloading them. The rich table with hyperlinks to the GCP console is a great touch for user experience.

However, there might be mypy errors when accessing Secret attributes. Would you consider adding type casting to resolve them? Something like:

- full_secret_name = secret.name
+ full_secret_name = cast(str, secret.name)

# And in the table.add_row:
        table.add_row(
            f"[link={secret_url}]{secret_name}[/link]",
-           "\n".join([f"{k}={v}" for k, v in secret.labels.items()]),
-           str(secret.create_time),
+           "\n".join([f"{k}={v}" for k, v in cast(dict, secret.labels).items()]),
+           str(cast(object, secret.create_time)),
        )

wdyt?


227-236: 🛠️ Refactor suggestion

Good extraction of secret writing logic!

The function properly handles writing secrets to files and setting appropriate permissions. However, there's still a potential mypy error related to accessing the name attribute. Consider:

- version_name = f"{secret.name}/versions/latest"
+ version_name = f"{cast(str, secret.name)}/versions/latest"

wdyt?


269-278: 🛠️ Refactor suggestion

Clean file path resolution logic!

The function properly handles the case when there's a filename label and falls back to a default. However, there might still be mypy errors related to accessing the labels attribute. Consider:

- if secret.labels and "filename" in secret.labels:
-     return secrets_dir / f"{secret.labels['filename']}.json"
+ labels = cast(dict, getattr(secret, "labels", {}))
+ if labels and "filename" in labels:
+     return secrets_dir / f"{labels['filename']}.json"

wdyt?

🧹 Nitpick comments (2)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (2)

19-47: Excellent structural improvements with imports!

The conditional imports for Google Secret Manager with type annotations are a good approach. It allows for graceful failure handling when dependencies are missing.

However, I notice you're using # type: ignore in several places. Would a TypeVar approach be more elegant for handling the Secret type? Something like:

from typing import TypeVar, cast, Any

# Define a generic type for Secret when the module is not loaded
SecretType = TypeVar("SecretType", bound=Any)

# Use SecretType in type annotations instead of string literals + type: ignore

wdyt?


51-51: Minor type annotation fix needed

The type ignore comment works, but it might be better to use an explicit type annotation:

- help=__doc__.replace("\n", "\n\n"),  # Render docstring as help text (markdown) # type: ignore
+ help=str(__doc__).replace("\n", "\n\n"),  # Render docstring as help text (markdown)

This way you explicitly convert the docstring to string, which should satisfy the type checker without needing to ignore it.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d6d0764 and d250de1.

📒 Files selected for processing (1)
  • airbyte_cdk/cli/airbyte_cdk/_secrets.py (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (4)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (4)

89-125: Great refactoring of the fetch command!

The restructured approach using helper functions makes the code much more maintainable and easier to understand. Breaking down the operation into logical steps (client creation, connector resolution, directory setup, secret fetching, and writing) improves readability significantly.

I particularly like the clear error message when no secrets are found for a connector.


201-225: Well-structured secret fetching logic!

Good implementation of the secret fetching function with proper filtering by connector label. The filter string construction is clean and the type annotations help with understanding the function signature.

I like that you're returning an explicit list comprehension for clarity.


258-265: Good secrets directory setup logic!

I like how you've ensured that:

  1. The secrets directory exists
  2. A .gitignore file is created to prevent accidental secret commits

This is an important security practice for handling sensitive information.


280-302: Secure credential handling implementation!

The client creation function properly:

  1. Checks for required dependencies
  2. Verifies environment credentials are present
  3. Creates the client with appropriate error messaging

This approach keeps sensitive credentials in environment variables rather than hardcoded, which is a security best practice.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (2)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (2)

282-285: 🛠️ Refactor suggestion

Use cast when building the version path to avoid type errors, wdyt?

-    version_name = f"{secret.name}/versions/latest"
+    version_name = f"{cast(str, getattr(secret, 'name', ''))}/versions/latest"

Without this, mypy flags Secret? has no attribute "name", and runtime may raise when the SDK is unavailable.


323-327: 🛠️ Refactor suggestion

⚠️ Potential issue

Guard against missing labels on the Secret handle, wdyt?

secret.labels can be None; dereferencing it unconditionally will blow up. A small refactor also fixes the mypy errors:

-    if secret.labels and "filename" in secret.labels:
-        return secrets_dir / f"{secret.labels['filename']}.json"
+    labels = cast(dict, getattr(secret, "labels", {}))
+    if "filename" in labels:
+        return secrets_dir / f"{labels['filename']}.json"
🧹 Nitpick comments (2)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (2)

41-45: Drop the unused numpy.isin import to trim dependencies, wdyt?

isin is never referenced in this module. Keeping it:

  1. Pulls numpy into the import graph (large, optional dep).
  2. Triggers linters for “unused import”.

A one-line diff removes the noise:

-from numpy import isin

414-415: Log response text instead of raw bytes for readability (and to fix str-bytes-safe), wdyt?

-        logger.error(f"Failed to fetch spec mask: {response.content}")
+        logger.error("Failed to fetch spec mask: %s", response.text)

This avoids the b'...' prefix and satisfies the linter.

🧰 Tools
🪛 GitHub Actions: Linters

[error] 414-414: If x = b'abc' then f"{x}" or "{}".format(x) produces "b'abc'", not "abc". If this is desired behavior, use f"{x!r}" or "{!r}".format(x). Otherwise, decode the bytes [str-bytes-safe]

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d250de1 and c11e1ec.

📒 Files selected for processing (1)
  • airbyte_cdk/cli/airbyte_cdk/_secrets.py (4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/cli/airbyte_cdk/_secrets.py

[error] 380-380: Missing type parameters for generic type "list" [type-arg]


[error] 414-414: If x = b'abc' then f"{x}" or "{}".format(x) produces "b'abc'", not "abc". If this is desired behavior, use f"{x!r}" or "{!r}".format(x). Otherwise, decode the bytes [str-bytes-safe]

⏰ Context from checks skipped due to timeout of 90000ms (9)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Analyze (python)
  • GitHub Check: SDM Docker Image Build

@aaronsteers aaronsteers changed the title feat(cli): add secrets list command feat(cli): add secrets list command and github ci secrets masking Apr 30, 2025
@aaronsteers aaronsteers changed the title feat(cli): add secrets list command and github ci secrets masking feat(cli): add secrets list CLI command and github ci secrets masking Apr 30, 2025
aaronsteers and others added 2 commits April 29, 2025 18:00
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (3)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (3)

238-246: Secret.labels may be None, and mypy still complains – shall we cast defensively?

Accessing secret.labels.items() will raise AttributeError when labels are None, and linters keep flagging attr-defined. We previously discussed using cast/getattr; the current code still uses direct access. Re-applying that change would address both runtime and static issues. WDYT?


283-285: Same issue for secret.name – safer with cast

secret.name is optional when the SDK isn’t installed, so wrapping with cast(str, getattr(secret, "name", "")) prevents AttributeError and quells mypy.


324-327: secret.labels access repeats the earlier mypy/runtime risk

If labels is None, the first condition short-circuits, but secret.labels['filename'] will still explode. Moving the labels to a local variable via cast(dict, getattr(secret, "labels", {})) avoids this pitfall.

🧹 Nitpick comments (2)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (2)

93-99: Flag already implies bool – can we drop the explicit type=bool?

click.option(..., is_flag=True) automatically makes the parameter a bool.
Specifying type=bool is redundant and can confuse Click’s help-text (it starts showing [TEXT] instead of a simple flag). WDYT about simplifying?

-@click.option(
-    "--print-ci-secrets-masks",
-    help="Print GitHub CI mask for secrets.",
-    type=bool,
-    is_flag=True,
-    default=False,
-)
+@click.option(
+    "--print-ci-secrets-masks",
+    help="Print GitHub CI mask for secrets.",
+    is_flag=True,
+    default=False,
+)

412-419: Network failure in _get_spec_mask will raise – want a graceful fallback?

If the YAML endpoint is unreachable, we log an error but still attempt to parse response.content, which will raise and abort the CLI. Would it be safer to return an empty list (and thus mask nothing) instead of crashing? Something like:

     if not response.ok:
         logger.error(f"Failed to fetch spec mask: {response.content.decode('utf-8')}")
-    try:
-        return cast(list[str], yaml.safe_load(response.content)["properties"])
-    except Exception as e:
-        logger.error(f"Failed to parse spec mask: {e}")
-        raise
+        return []
+    try:
+        return cast(list[str], yaml.safe_load(response.content)["properties"])
+    except Exception as e:
+        logger.error(f"Failed to parse spec mask: {e}")
+        return []

This keeps the command functional even when offline, at the cost of no masking. WDYT?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c11e1ec and f2e72b0.

📒 Files selected for processing (1)
  • airbyte_cdk/cli/airbyte_cdk/_secrets.py (4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (1)
airbyte_cdk/cli/airbyte_cdk/_util.py (2)
  • resolve_connector_name (46-69)
  • resolve_connector_name_and_directory (8-43)
⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: SDM Docker Image Build

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (2)

320-326: ⚠️ Potential issue

Same optional-attribute pattern in _get_secret_filepath

For consistency (and to silence the twin mypy error here), could we apply the same casting?

-    if secret.labels and "filename" in secret.labels:
-        return secrets_dir / f"{secret.labels['filename']}.json"
+    labels = cast(dict, getattr(secret, "labels", {}))
+    if labels and "filename" in labels:
+        return secrets_dir / f"{labels['filename']}.json"

This keeps runtime identical and removes the type-checker noise—wdyt?


236-245: ⚠️ Potential issue

mypy still complains about Secret attributes – cast to quiet the checker?

The earlier bot suggestions haven’t been applied, so secret.name, secret.labels, and secret.create_time still trigger attr-defined errors when the GSM SDK is optional.
Casting keeps runtime behaviour unchanged while satisfying static analysis.

-        full_secret_name = secret.name
+        full_secret_name = cast(str, getattr(secret, "name", ""))
 ...
-            "\n".join([f"{k}={v}" for k, v in secret.labels.items()]),
-            str(secret.create_time),
+            "\n".join([f"{k}={v}" for k, v in cast(dict, getattr(secret, "labels", {})).items()]),
+            str(getattr(secret, "create_time", "")),

Would this adjustment make sense to you?

🧹 Nitpick comments (1)
airbyte_cdk/cli/airbyte_cdk/_secrets.py (1)

407-411: Tiny clean-up: redundant list allocation in _is_secret_property

any([...]) eagerly builds a list; the generator expression version is leaner and easier to read:

-    names_to_mask: list[str] = _get_spec_mask()
-    if any([mask.lower() in property_name.lower() for mask in names_to_mask]):
-        return True
-
-    return False
+    names_to_mask = _get_spec_mask()
+    return any(mask.lower() in property_name.lower() for mask in names_to_mask)

Pure nit—feel free to ignore if you prefer the current style.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2e72b0 and d23bbb9.

📒 Files selected for processing (1)
  • airbyte_cdk/cli/airbyte_cdk/_secrets.py (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Analyze (python)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: SDM Docker Image Build
  • GitHub Check: Pytest (Fast)

@aaronsteers aaronsteers requested review from dbgold17 and bnchrch April 30, 2025 02:24
@aaronsteers aaronsteers requested a review from bazarnov April 30, 2025 02:25
Copy link
Contributor

@bazarnov bazarnov left a comment

Choose a reason for hiding this comment

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

Left few security concerns, otherwise looks great to me!

@aaronsteers aaronsteers enabled auto-merge (squash) April 30, 2025 14:49
@aaronsteers aaronsteers merged commit 4d04ed5 into main Apr 30, 2025
19 of 27 checks passed
@aaronsteers aaronsteers deleted the feat/cli/add-secrets-list-command branch April 30, 2025 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants