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

[MRG] Call parent preassemble scripts methods #752

Merged
merged 2 commits into from Jul 23, 2019

Conversation

@betatim
Copy link
Member

betatim commented Jul 23, 2019

Without this we don't collect the files that other build packs need to
copy them over to the container in the pre-assemble stage.

Needs a test to stop it from coming back.

closes #751

betatim added 2 commits Jul 23, 2019
Without this we don't collect the files that other build packs need to
copy them over to the container in the pre-assemble stage.
@betatim betatim changed the title [WIP] Call parent preassemble scripts methods [MRG] Call parent preassemble scripts methods Jul 23, 2019
@choldgraf

This comment has been minimized.

Copy link
Collaborator

choldgraf commented Jul 23, 2019

I see you've added a test now?

@choldgraf

This comment has been minimized.

Copy link
Collaborator

choldgraf commented Jul 23, 2019

I see you've added a test now? Or do you wanna add more?

@betatim

This comment has been minimized.

Copy link
Member Author

betatim commented Jul 23, 2019

Yes I have. Forgot to update the top comment

@choldgraf

This comment has been minimized.

Copy link
Collaborator

choldgraf commented Jul 23, 2019

This seems good to me - the only thing that seems a bit strange is the chain of sub-classing we've got going on. The RBuildPack inherits from the PythonBuildPack, which inherits from the CondaBuildPack, and so on. I don't see any reason that this PR should be the one to change that pattern, but just something I noticed that made it harder to figure out what was happening in that super() call.

@choldgraf choldgraf merged commit bf29f66 into jupyter:master Jul 23, 2019
5 checks passed
5 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
codecov/patch 100% of diff hit (target 20%)
Details
codecov/project 90.89% (+0%) compared to 6c5c09b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@betatim betatim deleted the betatim:fix-preassemble-r-py-combo branch Jul 23, 2019
@betatim

This comment has been minimized.

Copy link
Member Author

betatim commented Jul 23, 2019

CondaBuildPack is the actual "base buildpack" because we use it to install conda which we use to run the notebook server. This is why everything (or pretty much everything) inherits from the conda buildpack. Does that make sense?

(and it sometimes surprises me as well that the PythonBuildPack inherits from the conda one, but I think it makes sense. Maybe different names would help?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.