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

Install Oracle Instant Client #29

Merged
merged 6 commits into from
Jun 12, 2020
Merged

Conversation

calumabarnett
Copy link
Member

This PR will install Oracle Instant Client (19.6.0.0.0). This will enable analysts to directly query Oracle databases using the cx_Oracle package. This is in support of an interim measure to provide access to the Delius database on the Analytical Platform while work on the pipeline is ongoing.

Copy link
Contributor

@xoen xoen left a comment

Choose a reason for hiding this comment

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

As said in Slack, with the limited knowledge of this Oracle thing this seems fine to me.

I'd advice you to try this on your own JupyterLab instance for awhile before mass-releasing to all users as there are few changes in the environment which could cause compatibility problems to the users (e.g. the base image change so probably quite few system libraries and potentially version of Python and conda, etc...so proceed with caution ☺️ )

@calumabarnett
Copy link
Member Author

calumabarnett commented Jun 10, 2020

The image has built OK and I've managed to deploy it to my own JupyterLab instance with a couple of small tweaks to the Helm chart and it looks to be working on the face of it, though I'm probably not a good person to test further.

Once the database connection is created I can deploy it some other users for further testing.

@davidread
Copy link
Contributor

@calumabarnett We talked about making this image with the Oracle client available only to limited users, who are manually given this image - see https://docs.google.com/document/d/1ISoxf47qF3l0VqBo3d-yY4SekCQoBmmGiqlnVowtoaE/edit#
How about we achieve this in a similar way to the all-spark image, by putting the Dockerfile in a separate directory in this repo? It just needs someone to setup Quay to build it and then it's available for users to specify it in their Deployment.

@calumabarnett
Copy link
Member Author

Thanks @davidread – sounds like a sensible approach. I've reverted the datascience-notebook Dockerfile, moved the updated Dockerfile to a new directory called oracle-datascience-notebook and created a new Quay repo of the same name to build from this directory.

@calumabarnett calumabarnett requested review from davidread and xoen June 11, 2020 15:51
Copy link
Contributor

@davidread davidread left a comment

Choose a reason for hiding this comment

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

👍 Seems pretty straightforward. Nice to have these oracle bits together in the Dockerfile. The rest is a copy of the other Dockerfile, but I think it's worth a bit of DRY to have a separate image built, so that this functionality is not made available to most users.

@davidread
Copy link
Contributor

That build error about missing [jobs] suggests the syntax in the https://github.com/ministryofjustice/analytics-platform-jupyter-notebook/blob/master/circle.yml needs a little update, if you can work that out too before merge?

@xoen
Copy link
Contributor

xoen commented Jun 12, 2020

Actually the comment about the repetition as I wonder whether this Dockerfile could use the standard JupyterLab image as BASE and install this on top of that to make it clear it's the only difference.

@calhutchison sorry I didn't think/suggest about this before 😬

Basically it's similar to what they do in the official R/Jupyter images where they have few common/base images and they have more specialised images which add/inherit on top of these images to add functionalities.

It makes a lot of sense as it clear what's special about this docker image as its Dockerfile would contain only the difference.

Just a thought.

@calumabarnett
Copy link
Member Author

That build error about missing [jobs] suggests the syntax in the https://github.com/ministryofjustice/analytics-platform-jupyter-notebook/blob/master/circle.yml needs a little update, if you can work that out too before merge?

@davidread It's a very old v1 Circle config. I'm not sure it's used/needed anymore anyway as all PRs trigger a build on Quay directly. Perhaps what's needed is just to remove it?

@davidread davidread mentioned this pull request Jun 12, 2020
@davidread
Copy link
Contributor

@calumabarnett Makes sense. I've removed the Circle config in this PR: #31

@davidread
Copy link
Contributor

This is good to merge now I think

@calumabarnett
Copy link
Member Author

@xoen that does make sense – I've quickly tried to build from the base image but have run into some errors with apt. I'm sure they could be fixed but given we have something working and I don't think the benefits are huge, I'm inclined to stick with things as they are.

@calumabarnett calumabarnett merged commit 02a76b2 into master Jun 12, 2020
@calumabarnett calumabarnett deleted the cb--oracle-instant-client branch June 12, 2020 13:05
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