Skip to content
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

airbyte-ci: Pass env vars to poetry container in test command #34288

Merged
merged 7 commits into from
Jan 18, 2024

Conversation

flash1293
Copy link
Contributor

This PR allows to pass through environment variables of the host sytem to the container executing tests:

airbyte-ci test my_package --pass-env-var="GCM_CREDENTIALS"

Behavior:

  • Multiple env vars can be passed in
  • As they might be confidental, they are treated as secrets as far as dagger is concerned
  • If they are not set on the host system, a descriptive error is thrown

@flash1293 flash1293 requested a review from a team January 16, 2024 14:16
Copy link

vercel bot commented Jan 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Jan 18, 2024 11:35am

Copy link

sweep-ai bot commented Jan 16, 2024

Apply Sweep Rules to your PR?

  • Apply: All new business logic should have corresponding unit tests.
  • Apply: Refactor large functions to be more modular.
  • Apply: Add docstrings to all functions and file headers.

This is an automated message generated by Sweep AI.

@flash1293
Copy link
Contributor Author

@aaronsteers we can use this to pass the GCM credentials to the airbyte-lib test to download secrets for integration tests using external resources.

Copy link

sweep-ai bot commented Jan 16, 2024

Sweeping

Fixing PR: track the progress here.

I'm currently fixing this PR to address the following:

[Sweep GHA Fix] The GitHub Actions run failed with the following error logs:



[!CAUTION]

An error has occurred: Cmd('git') failed due to: exit code(128)
cmdline: git clone -v --branch=flash1293/pass-env-var -- https://:@github.com/airbytehq/airbyte.git /tmp/cache/repos/airbytehq/airbyte/base/flash1293--pass-env-var
stderr: 'fatal: could not create work tree dir '/tmp/cache/repos/airbytehq/airbyte/base/flash1293--pass-env-var': No space left on device
' (tracking ID: 1f88dff9e8)

Copy link

sweep-ai bot commented Jan 16, 2024

Sweeping

Fixing PR: track the progress here.

I'm currently fixing this PR to address the following:

[Sweep GHA Fix] The GitHub Actions run failed with the following error logs:



[!CAUTION]

An error has occurred: Cmd('git') failed due to: exit code(128)
cmdline: git clone -v --branch=flash1293/pass-env-var -- https://:@github.com/airbytehq/airbyte.git /tmp/cache/repos/airbytehq/airbyte/base/flash1293--pass-env-var
stderr: 'fatal: could not create work tree dir '/tmp/cache/repos/airbytehq/airbyte/base/flash1293--pass-env-var': No space left on device
' (tracking ID: e82a4b0b13)

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Nice improvement!
I'm wondering what the more convenient:

  1. Explicitly passing env var like in docker run with --env MY_ENV_VAR='my-value'
  2. Passing already set env vars

1 has the benefit of not pre-setting the env var on your host
2 has the benefit of not passing secret as CLI input

Please bump the package version in pyproject.toml and README.md.

Comment on lines 131 to 133
for var in pipeline_context.params["pass_env_var"]:
secret = dagger_client.set_secret(var, os.environ[var])
test_container = test_container.with_secret_variable(var, secret)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we set this at a more higher level during container declaration?
Let's say we want to override DOCKER_VERSION or if poetry install behavior depends on some env var value? The drawback of setting this earlier is that it'll clear downstream layers (re-running poetry install on env var changes).

@@ -44,6 +52,13 @@ async def run_poetry_command(container: dagger.Container, command: str) -> Tuple
help="The poetry run command to run.",
required=True,
)
@click.option(
"--pass-env-var",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"--pass-env-var",
"--pass-env-var", "-e", "passed_env_vars"

To have a shorter -e option and a context object key named with plural passed_env_vars

Copy link
Collaborator

@aaronsteers aaronsteers left a comment

Choose a reason for hiding this comment

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

Looks good for my part!

@flash1293 flash1293 merged commit 9b4ae62 into master Jan 18, 2024
24 checks passed
@flash1293 flash1293 deleted the flash1293/pass-env-var branch January 18, 2024 13:57
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
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.

3 participants