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

Fixes mirror bug when originalHost is excluded #569

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

TisVictress
Copy link
Contributor

Also adds tests to makes sure that url path is complete


})
})

context("without originalHost placeholder", func() {
Copy link
Member

Choose a reason for hiding this comment

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

what does this mean?

Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on what the bug that you're solving is?

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 user can use {originalHost} as a placeholder to include the original url path when specifying a mirror. For example, if the original url is https://artifacts.paketo.io/nginx_1.24.0_linux_x64_jammy_fa1dfe46.tgz and the mirror is https://user:pass@local-mirror.example.com/buildpacks-dependencies/{originalHost}, then this will translate to https://user:pass@local-mirror.example.com/buildpacks-dependencies/artifacts.paketo.io/nginx_1.24.0_linux_x64_jammy_fa1dfe46.tgz. But if the placeholder is excluded, then the mirror translates to https://user:pass@local-mirror.example.com/buildpacks-dependencies/nginx_1.24.0_linux_x64_jammy_fa1dfe46.tgz

@TisVictress
Copy link
Contributor Author

TisVictress commented Apr 29, 2024

@sophiewigmore In the initial change, the mirror url was not properly constructed and did not include the actual dependency archive. For instance, with the following:
original: https://artifacts.paketo.io/nginx_1.24.0_linux_x64_jammy_fa1dfe46.tgz
mirror: https://user:pass@local-mirror.example.com/buildpacks-dependencies
...it would not append the actual tgz file to be downloaded. The result instead was https://user:pass@local-mirror.example.com/buildpacks-dependencies/artifacts.paketo.io
This also happened with special hostnames:
original: https://github.com/nginx/nginx_1.24.0_linux_x64_jammy_fa1dfe46.tgz
mirror: https://mirror.example.org/public-github
...the result would incorrectly be https://mirror.example.org/public-github when it should be https://mirror.example.org/public-github/nginx/nginx_1.24.0_linux_x64_jammy_fa1dfe46.tgz

The initial change only worked correctly if the {originalHost} placeholder was used. This is due to line 35 of dependency_mirror.go. The path needed to be moved outside of the substitution.

I also updated the tests because the test urls did not include an appropriate scheme and path

@sophiewigmore
Copy link
Member

This is helpful info! thank you

@sophiewigmore sophiewigmore added the semver:patch A change requiring a patch version bump label Apr 29, 2024
@sophiewigmore sophiewigmore enabled auto-merge (squash) April 29, 2024 14:37
@sophiewigmore sophiewigmore merged commit ce376b7 into paketo-buildpacks:v2 Apr 29, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants