-
Notifications
You must be signed in to change notification settings - Fork 360
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
the legacy buildpack can still be used in "binder" or ".binder" folders #932
Comments
I think it should not check for if self.binder_dir:
return False before checking the top-level Dockerfile contents to make sure legacy is never used if binder_dir is present. |
Should we add this special casing? Everywhere else in repo2docker we have the "rule" that "if there is a I'm wondering if there is a reason for repo owners wanting to keep the legacy |
tl;dr: I think the logic we have now is right, and we shouldn't make any changes.
Not quite, because The logic (excluding Legacy) is more like:
which results in the effective priority:
i.e. the presence of
I think special-casing legacy makes sense, since it is, somewhat by definition, a special case. The goal of the Legacy buildpack is to keep some fraction of old pre-repo2docker Binder repos working, and not use any of the newer repo2docker logic in that case. Stepping back, I think the logic for picking legacy should be:
So the question is really, in the presence of a top-level Dockerfile that looks like it was built for the old Binder, does the presence of a Case for using Legacy in this situation:
Case for not using Legacy in this situation:
Now that I think of it, our current logic makes more sense than the change I proposed for two reasons:
To make this change, we would be enabling a new partial transition from Binder v1 to repo2docker without dropping support for the old way, which hasn't been operation for a couple of years now. |
In #829 the legacy buildpack (
LegacyBinderDockerBuildPack
) is removed, but actually it can still be used in binder config folder, becausedetect
method only checks the Dockerfile at the root:https://github.com/jupyter/repo2docker/blob/8bbced7ded5a21b581f1f3846ffc9f87944ba799/repo2docker/buildpacks/legacy/__init__.py#L19-L23
was this on purpose? Or should we create a PR and change it to check
Dockerfile
inbinder_dir
?Btw I know this is not a realistic usecase, but if a repo contains
this will also throws error saying that legacy buildpack is removed, but actually it should pick python buildpack.
The text was updated successfully, but these errors were encountered: