-
Notifications
You must be signed in to change notification settings - Fork 90
Split code editor extension installation for different environments #916
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
base: main
Are you sure you want to change the base?
Conversation
# CodeEditor - create server, user data dirs | ||
mkdir -p /opt/amazon/sagemaker/sagemaker-code-editor-server-data /opt/amazon/sagemaker/sagemaker-code-editor-user-data \ | ||
&& chown $MAMBA_USER:$MAMBA_USER /opt/amazon/sagemaker/sagemaker-code-editor-server-data /opt/amazon/sagemaker/sagemaker-code-editor-user-data && \ | ||
mkdir -p /opt/amazon/sagemaker/sagemaker-code-editor-server-data /opt/amazon/sagemaker/sagemaker-ui-code-editor-server-data /opt/amazon/sagemaker/sagemaker-code-editor-user-data \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would also need a separate /opt/amazon/sagemaker/sagemaker-ui-code-editor-user-data right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no difference in how we're configuring the user-data
folder across environments - we can add later if needed, I've made minimal changes needed for now
&& extensionloc_ui=/opt/amazon/sagemaker/sagemaker-ui-code-editor-server-data/extensions && mkdir -p "${extensionloc_ui}" \ | ||
&& for ext in /etc/code-editor/extensions-sagemaker-ui/*.vsix; do \ | ||
echo "Installing sagemaker-ui extension ${ext}..."; \ | ||
sagemaker-code-editor --install-extension "${ext}" --extensions-dir "${extensionloc_ui}" --server-data-dir /opt/amazon/sagemaker/sagemaker-ui-code-editor-server-data --user-data-dir /opt/amazon/sagemaker/sagemaker-code-editor-user-data; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets have a separate user data dir as well to keep things clean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented above, we can add when needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is stored in this user data? Could it have some conflict or be overridden if different extension versions are used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This folder will contain the user level settings - https://code.visualstudio.com/docs/configure/settings#_user-settings. We do not save anything in this folder in sagemaker-distribution image
Since we are explicitly specifying the ...server-data/
dir for extensions, I do not think there's any link with the user-data-dir. Let's confirm all works as expected during our testing
|
||
persistent_settings_folder="${EBS_MOUNT_POINT}/sagemaker-code-editor-server-data" | ||
default_settings_folder="${EFS_MOUNT_POINT}/sagemaker-code-editor-server-data" | ||
persistent_ebs_server_data_folder="${EBS_MOUNT_PATH}/sagemaker-code-editor-server-data" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to sagemaker-ui-code-editor-server-data as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't that be backward incompatible ? this EBS folder could have content already - we should continue using the same folder in EBS
Description
[Provide a brief description of the changes]
Type of Change
Release Information
Does this change need to be included in patch version releases? By default, any pull requests will only be added to the next SMD image minor version release once they are merged in template folder. Only critical bug fix or security update should be applied to new patch versions of existed image minor versions.
If yes, please explain why:
[Explain the criticality of this change and why it should be included in patch releases]
How Has This Been Tested?
Pending testing - for initial review
Checklist:
Test Screenshots (if applicable):
Related Issues
[Link any related issues here]
Additional Notes
[Any additional information that might be helpful for reviewers]