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: repo build errors and vscode issues #252

Merged
merged 1 commit into from
May 14, 2021
Merged

Conversation

JessicaBarh
Copy link
Contributor

@JessicaBarh JessicaBarh commented May 10, 2021

Description

  • Fixes repo build errors (vulnerabilities and unpinned upstream image causing a dependency conflict)
  • Fixes vscode not showing in french and having broken python support

Resolves #249, #247, #240 also #257

Tests / Quality Checks

Automated Testing/build and deployment

  • Does the image pass CI successfully (build, pass vulnerability scan, and pass automated test suite)?
  • If new features are added (new image, new binary, etc), have new automated tests been added to cover these?
  • If new features are added that require in-cluster testing (e.g. a new feature that needs to interact with kubernetes), have you added the auto-deploy tag to the PR before pushing in order to build and push the image to ACR so you can test it in cluster as a custom image?

JupyterLab extensions

  • Are all extensions "enabled" (jupyter labextension list from inside the notebook)?

VS Code tests

  • Does VS Code open?
  • Can you install extensions?

Code review

  • Have you added the auto-deploy tag to your PR before your most recent push to this repo? This causes CI to build the image and push to our ACR, letting reviewers access the built image without having to create it themselves
  • Have you chosen a reviewer, attached them as a reviewer to this PR, and messaged them with the SHA-pinned image name for the final image to test (e.g. k8scc01covidacr.azurecr.io/machine-learning-notebook-cpu:746d058e2f37e004da5ca483d121bfb9e0545f2b)?

@JessicaBarh JessicaBarh added auto-deploy Trigger manual CI steps for this PR and removed auto-deploy Trigger manual CI steps for this PR labels May 10, 2021
@JessicaBarh
Copy link
Contributor Author

JessicaBarh commented May 11, 2021

New vulnerability found: CVE-2021-23334

according to the comments in this issue, it's a false positive and in the process of being revoked.

Vulnerability withdrawn GHSA-8v27-2fg9-7h62

@JessicaBarh JessicaBarh marked this pull request as ready for review May 11, 2021 17:47
Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@JessicaBarh JessicaBarh changed the title fix: remove pdfbox for CVE-2019-0228 fix: repo build errors (vulnerabilities, upstream image pinning) May 11, 2021
@JessicaBarh JessicaBarh added the auto-deploy Trigger manual CI steps for this PR label May 12, 2021
@Jose-Matsuda
Copy link
Contributor

There's some annoying behaviour happening right now with the French Language extension. It is installed, but vscode is not recognizing it as a "display language" option. Using the older version of the French extension did not work either.
image

I think the reason is the languagepacks.json not getting created properly in the /etc/share/code-server/ directory.
Compare our current
image

To what I get when I try running locally
image

I'll try this out in a bit, as well as maybe make it such that it would use the French file on startup (previously we just installed it and say the LANG variable had no impact on how code-server was starting up)

While not helpful right now, the folks at cdr are looking into doing this sort of thing automatically coder/code-server#3173

@ca-scribner
Copy link
Contributor

I was going to do a review but I'll hold off on this till Jose reports back :)

@Jose-Matsuda
Copy link
Contributor

Jose-Matsuda commented May 12, 2021

All right looks like we good would also close #240 also closes #257

@ca-scribner ca-scribner changed the title fix: repo build errors (vulnerabilities, upstream image pinning) fix: repo build errors and vscode issues May 13, 2021
Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

Added two suggested comments/changes to comments

Also pushed a change moving the libpdfbox-java uninstall up in the stack. I want CI to build successfully in case I broke something, so I haven't approved yet. Will approve when that is done

docker-bits/0_Rocker.Dockerfile Show resolved Hide resolved
Comment on lines 1 to 3
# as simply using the tag does not allow stability and reproducibility as these image tags are updated frequently.
# the BASE_VERSION pinned below corresponds to a git commit SHA for image jupyter/datascience-notebook:r-4.0.3
# Using the git-SHA is unique and immutable. it can be obtained by running `docker inspect repo/imagename:tag@digest`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# as simply using the tag does not allow stability and reproducibility as these image tags are updated frequently.
# the BASE_VERSION pinned below corresponds to a git commit SHA for image jupyter/datascience-notebook:r-4.0.3
# Using the git-SHA is unique and immutable. it can be obtained by running `docker inspect repo/imagename:tag@digest`
# Docker-stacks version tags (eg: `r-4.0.3`) are LIVE images that are frequently updated. To avoid unexpected
# image updates, pin to the docker-stacks git commit SHA tag.
# It can be obtained by running `docker inspect repo/imagename:tag@digest` or from
# https://github.com/jupyter/docker-stacks/wiki

@blairdrummond blairdrummond force-pushed the vulnerability-fix branch 2 times, most recently from 20449a4 to a16f165 Compare May 13, 2021 18:48
Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

team effort! :D lgtm

@ca-scribner
Copy link
Contributor

Leaving for @JessicaBarh to merge once tests pass. Jess please make sure to squash on merge (I'm terrible at remembering that...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-deploy Trigger manual CI steps for this PR
Projects
None yet
3 participants