-
Notifications
You must be signed in to change notification settings - Fork 62
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
Adding optional components flag for custom script #96
base: main
Are you sure you want to change the base?
Adding optional components flag for custom script #96
Conversation
startup_script/run.sh
Outdated
@@ -87,10 +92,25 @@ function cleanup() { | |||
rm ./init_actions.sh ./run.sh | |||
} | |||
|
|||
function is_version_at_least_2_3() { |
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.
Can we make this a generic function? and pass the value "2.3" ?
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.
The new version_ge
function from init actions might be good to copy here
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.
The problem is that here DATAPROC_IMAGE_VERSION is not present. So, I am extracting the image version from the image name itself.
…apture the version being used.
startup_script/run.sh
Outdated
function main() { | ||
wait_until_ready | ||
|
||
if [[ "${ready}" == "true" ]]; then | ||
run_startup_custom_script |
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.
let's rename to "run_install_optional_components_script"
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.
done
startup_script/run.sh
Outdated
|
||
function run_startup_custom_script() { | ||
if is_version_at_least "2.3" && [[ -n "$USER_DATAPROC_COMPONENTS" ]]; then | ||
source "${BDUTIL_DIR}/startup_optional_components.sh" |
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.
rename to "install_optional_components"
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.
done
custom_image_utils/args_parser.py
Outdated
type=_validate_components, | ||
required=False, | ||
help="""Optional Components to be installed with the image. | ||
Can be a comma-separated list of components, e.g., TRINO,JUPYTER.""" |
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.
let's change it from JUPYTER to some other component , as we are giving JUPYTER in the image
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.
done
"--optional-components", | ||
type=_validate_components, | ||
required=False, | ||
help="""Optional Components to be installed with the image. |
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.
I think, description can mention that, this option is applicable for images 2.3+
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.
done
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.
I have yet to exercise this change, but here are a few questions I have after review.
@@ -30,6 +30,7 @@ | |||
_FULL_IMAGE_URI = re.compile(r"^(https://www\.googleapis\.com/compute/([^/]+)/)?projects/([^/]+)/global/images/([^/]+)$") | |||
_FULL_IMAGE_FAMILY_URI = re.compile(r"^(https://www\.googleapis\.com/compute/([^/]+)/)?projects/([^/]+)/global/images/family/([^/]+)$") | |||
_LATEST_FROM_MINOR_VERSION = re.compile(r"^(\d+)\.(\d+)-((?:debian|ubuntu|rocky)\d+)$") | |||
_VALID_OPTIONAL_COMPONENTS = ["HIVE_WEBHCAT", "ZEPPELIN", "TRINO", "RANGER", "SOLR", "FLINK", "DOCKER", "HUDI", "ICEBERG", "PIG"] |
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.
Is there a reason JUPYTER is not listed here?
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.
@IshaAg can help with the reasoning for this
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.
Jupyter is part of core image now
Can you please update the description to clearly state what the change will do? |
Done |
Adding a flag which takes optional components as an argument and installs it during image creation time. This feature is only supported for 2.3 and above images.