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 parsing docker image when the full image name contains registry port #349

Closed
wants to merge 12 commits into from

Conversation

ogusak
Copy link
Contributor

@ogusak ogusak commented Aug 15, 2024

What

The existing docker image parsing assumes only a single : delimiter in the image name which is used to denote image tag in the image name. However, in a general case, the full image name may contain registry host with port number, which is also delimited with :.

See full description here

How

Parse full image name into version and image name based on the last occurrence of : delimiter

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

@CLAassistant
Copy link

CLAassistant commented Aug 15, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

Your branch is not currently up-to-date with main. Please update your branch before attempting to snapshot your PR.

@ogusak ogusak changed the title 🐛 Fix parsing docker image when when the full image name contains registry port 🐛 Fix parsing docker image when the full image name contains registry port Aug 16, 2024
@marcosmarxm
Copy link
Member

Thanks for the fix @ogusak I'm gonna ask the platform team to review this during the week.

@ogusak
Copy link
Contributor Author

ogusak commented Aug 22, 2024

@marcosmarxm - could you check with your team please on reviewing this PR - thanks!

@ogusak
Copy link
Contributor Author

ogusak commented Aug 29, 2024

Hi @marcosmarxm - could someone take a look at this PR. Thanks!

@marcosmarxm
Copy link
Member

Can someone from @airbytehq/platform-compose / @airbytehq/platform-move take a look into this small contribution?

@marcosmarxm
Copy link
Member

Sorry the delay @ogusak. @jdpgrailsdev can you help reviewing this contribution, thanks!

Copy link
Contributor

@jdpgrailsdev jdpgrailsdev left a comment

Choose a reason for hiding this comment

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

:shipit:

@ogusak
Copy link
Contributor Author

ogusak commented Sep 10, 2024

Thanks a lot for taking a look, @jdpgrailsdev !

@jdpgrailsdev
Copy link
Contributor

/create-oss-pr

@jdpgrailsdev
Copy link
Contributor

@ogusak I'm not sure why the CI actions didn't catch this, but there appears to be a failing test in this PR:

ConnectorApmSupportHelperTest > testExtractAirbyteVersionFromBlankImageName() FAILED
    java.lang.NullPointerException: Cannot invoke "String.lastIndexOf(String)" because "image" is null
        at io.airbyte.workers.helper.ConnectorApmSupportHelper.getImageName(ConnectorApmSupportHelper.java:77)
        at io.airbyte.workers.helper.ConnectorApmSupportHelperTest.testExtractAirbyteVersionFromBlankImageName(ConnectorApmSupportHelperTest.java:49)

@ogusak
Copy link
Contributor Author

ogusak commented Sep 16, 2024

@jdpgrailsdev - thanks for surfacing that error! Fixed it in the latest commit where I restored the original check for null/empty image name in ConnectorApmSupportHelper.

@jdpgrailsdev
Copy link
Contributor

/create-oss-pr

@jdpgrailsdev
Copy link
Contributor

@ogusak This change has been merged into our internal repo and should shortly be replicated to this repo. It will be included in the next release. Let me know if you have any questions. Thanks again for the submission.

@ogusak
Copy link
Contributor Author

ogusak commented Sep 17, 2024

Thanks for letting me know, @jdpgrailsdev! Looking forward to deploying the new Airbyte version with the fix!

@jdpgrailsdev
Copy link
Contributor

Closing, as this has been merged internally and replicated to this repository.

@jdpgrailsdev
Copy link
Contributor

@ogusak Release v0.64.7 contains this fix.

@ogusak
Copy link
Contributor Author

ogusak commented Sep 18, 2024

Thank a lot, @jdpgrailsdev !

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.

4 participants