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

Pass docker hub credentials to airbyte-ci's bump_version command #35802

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

ambirdsall
Copy link
Contributor

What

The bump_version command, as part of its process to bump the connector version tag in its metadata.yaml file, validates the metadata. The pipeline step that does this requires the docker hub credentials to be passed in (should it? That's a separate question). The bump_version command does not pass them in when creating the context for the pipeline's execution. This PR changes that.

Airbyters can read this slack thread for additional context.

🚨 User Impact 🚨

People bumping versions with airbyte-ci connectors --name $connector_name bump_version $args will actually succeed as long as they don't mess up their CLI args.

@ambirdsall ambirdsall requested a review from a team as a code owner March 4, 2024 22:19
Copy link

vercel bot commented Mar 4, 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 Mar 6, 2024 10:18pm

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.

Thanks for the fix!
These credentials are required because MetadataValidation happens during the execution flow (to validate that the new metadata file is correct).
MetadataValidation checks that the base image declared in it exists on dockerhub, and to do so it has to call dockerhub api.
If not logged in, DockerHub API returns a 403 and not a 404 when the image is not found... Hence the requirement for credentials.
We could decide to remove this validation in metadata ...

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.

Can you please bump the version in pipelines/pyproject.toml and add a changelog entry in README.md?

@ambirdsall ambirdsall force-pushed the amb/fix-bump_version-command-in-airbyte-ci branch from 9ac4997 to 74a2c7c Compare March 6, 2024 22:14
@ambirdsall ambirdsall merged commit 8ab77d4 into master Mar 6, 2024
31 checks passed
@ambirdsall ambirdsall deleted the amb/fix-bump_version-command-in-airbyte-ci branch March 6, 2024 22:59
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