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

Small fixes to python cdk publishing #36945

Merged
merged 5 commits into from
Apr 10, 2024
Merged

Small fixes to python cdk publishing #36945

merged 5 commits into from
Apr 10, 2024

Conversation

erohmensing
Copy link
Contributor

@erohmensing erohmensing commented Apr 9, 2024

  • Use correct version reference in commit message
  • Use correct version reference in changelog message
    • Backfill changelog messages
  • Add loop with timeout to keep trying to get the newly published CDK version until it is available
  • Update error messaging when source-declarative-manifest cannot be published

@erohmensing erohmensing requested a review from a team as a code owner April 9, 2024 22:00
Copy link

vercel bot commented Apr 9, 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 Apr 10, 2024 2:58pm

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Apr 9, 2024
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @erohmensing and the rest of your teammates on Graphite Graphite

@erohmensing erohmensing requested a review from maxi297 April 9, 2024 22:02
Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

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

LGTM!

run: |
cd airbyte-integrations/connectors/source-declarative-manifest
# --no-cache is used to ensure the latest version is fetched. Let's see if we need to add a wait...
poetry add airbyte-cdk==${{needs.bump-version.outputs.new_cdk_version}} --no-cache
while true; do
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems very odd. If it fails, will there be a good error message to indicate the failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it continues to fail, the github action will timeout. Then we will get the standard slack error message of us not being able to update the source-declarative-manifest version

Are you worried about it failing to actually add the dependency (compared to if it can't resolve it in time)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the latter. Like, I don't understand why with pip we were fine and now we need this. When it feels, it would be nice to have an actionnable error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see! manage.sh took over 5 minutes of processing before getting to the "install the newly published version of the cdk into the docker image" step of the job that happened directly after the cdk publish to pypi step. This gave it time to become available.

This job by comparison gets to the installation step in 42 seconds after the publish job, which doesn't seem to be quite enough time (sometimes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The output of both the failures and the successes shoudl still print to the GHA logs. I'll add an echo about the retry to make it make more sense

@erohmensing erohmensing merged commit ed8cebe into master Apr 10, 2024
26 checks passed
@erohmensing erohmensing deleted the ella/smfix branch April 10, 2024 15:10
Copy link
Contributor Author

Merge activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants