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

fix: use correct icon url path in metadata publish #38247

Merged
merged 6 commits into from
May 16, 2024
Merged

Conversation

erohmensing
Copy link
Contributor

@erohmensing erohmensing commented May 15, 2024

What

Fix an issue where connector publishing would not upload the icon to the metadata service. In this PR, we started to make modifications to the metadata file before we upload it. This leads to us rewriting the modified file to a tempfile path. However, since we are inferring the doc url based on the metadata file path, when we update the filepath to point to the new temp file, we lose the pointer to the doc url

See context thread here

How

Get the icon URL path before modifying the metadata file.

Also log the things we don't upload as well as the things that we do, so that this doesn't take like 4 hours to track down next time 😅

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

@erohmensing erohmensing requested a review from a team as a code owner May 15, 2024 23:52
Copy link

vercel bot commented May 15, 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 May 16, 2024 4:48pm

Copy link
Contributor Author

erohmensing commented May 15, 2024

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 bnchrch May 15, 2024 23:55
@erohmensing erohmensing changed the base branch from ella/invalidsteps to master May 15, 2024 23:59
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 🙏 Can you please bump the package version in pyproject.toml?

Copy link
Contributor

@bnchrch bnchrch left a comment

Choose a reason for hiding this comment

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

Approved! Sorry to be the cause of this goose chase!

@erohmensing erohmensing merged commit 01acc55 into master May 16, 2024
29 checks passed
@erohmensing erohmensing deleted the ella/iconurl branch May 16, 2024 17:04
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