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(Remote Desktop): repair quarto, vscode extensions #661

Merged
merged 67 commits into from
Oct 9, 2024
Merged

Conversation

jacek-dudek
Copy link
Contributor

Description

What your PR adds/fixes/removes

Tests / Quality Checks

Are there breaking changes?

Ask yourself the next question;

  • Do we want to maintain the previous image from which we had to do breaking changes from?

If no, then carry on. If yes, there is a breaking change and we want to maintain the previous image do the following

  • Create a new branch for the current version (ex v1) based off the current master/main branch
  • Increment the tag in the CI for pushes to master/main (v1 to v2)
  • Change the CI that on pushes to the newly created "v1" branch (the name of the newly created branch we want to maintain is) it will push to the ACR.

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 on the dev cluster (e.g. k8scc01covidacrdev.azurecr.io/jupyterlab-cpu:746d058e2f37e004da5ca483d121bfb9e0545f2b)?

@jacek-dudek jacek-dudek added the auto-deploy Trigger manual CI steps for this PR label Sep 17, 2024
jacekzbigniewdudek added 28 commits September 17, 2024 19:50
…e is a language pack sub-directory somewhere in there.
@jacek-dudek jacek-dudek linked an issue Oct 8, 2024 that may be closed by this pull request
@jacek-dudek jacek-dudek changed the title Cosmetic change to initiate build action on github. Changes to dockerfile fragments to update remote desktop image and get it building successfully. Oct 8, 2024
@jacek-dudek jacek-dudek linked an issue Oct 8, 2024 that may be closed by this pull request
3 tasks
@jacek-dudek
Copy link
Contributor Author

jacek-dudek commented Oct 8, 2024

This pull request is ready to be reviewed. Note again that the test phase steps are commented out from the .github workflow file because there was a JSONDecodeError thrown by one of the python test scripts. So omitting the test phase will apply to the other images.

@Souheil-Yazji
Copy link
Contributor

Souheil-Yazji commented Oct 9, 2024

@jacek-dudek A couple of notes regarding the pr:

  1. each RUN statement adds a layer to the image, and a step to the build which causes 1. longer build time 2. worse cache management 3. ballooning of image size
  2. You should only use ENV when that value will be used at container runtime
  3. if you are building the dockerfile as user=root there should be no need for sudo
  4. WORKDIR creates a layer, same issue as point 1, should be a mkdir in case of tmp dirs
  5. node and npm are already installed in 6_remote_desktop , yet you are reinstalling them in the command right after and archiving the ones we just installed, then restoring the old ones
  6. Did you test that everything runs fine now?

@jacek-dudek
Copy link
Contributor Author

I concatenated the various RUN directives and reverted the changes that were breaking things again.

@Souheil-Yazji Souheil-Yazji changed the title Changes to dockerfile fragments to update remote desktop image and get it building successfully. Fix(Remote Desktop): repair quarto, vscode extensions Oct 9, 2024
@Souheil-Yazji Souheil-Yazji merged commit dd7e7e2 into master Oct 9, 2024
6 checks passed
@Souheil-Yazji Souheil-Yazji deleted the aaw-660 branch October 9, 2024 18:49
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
Development

Successfully merging this pull request may close these issues.

Fix Remote Desktop Build Issue
2 participants