Skip to content

Conversation

@doxiao
Copy link
Member

@doxiao doxiao commented Sep 28, 2018

  1. Moved out the scripts that are previously inside the create domain configmap into separate files; those files can be replaced by customers' scripts.
  2. Added inputs properties to customize the behavior
  3. Renamed podDomainRootDir to domainPVMountPath
  4. Renamed weblogicImage to image, and weblogicImagePullSecretName to imagePullSecretName
  5. Use env variables instead of string substitution on in the build-in create domain scripts.

(note #4 and #5 addressed review comments)

@rjeberhard @moreaut @tbarnes-us

Signed-off-by: doxiao <dongbo.xiao@oracle.com>
Signed-off-by: doxiao <dongbo.xiao@oracle.com>
Signed-off-by: doxiao <dongbo.xiao@oracle.com>
Signed-off-by: doxiao <dongbo.xiao@oracle.com>
Signed-off-by: doxiao <dongbo.xiao@oracle.com>

# create the configmap and label it properly
kubectl create configmap ${domainUID}-create-weblogic-sample-domain-job-cm -n $namespace --from-file $externalFilesTmpDir
kubectl label configmap ${domainUID}-create-weblogic-sample-domain-job-cm -n $namespace weblogic.resourceVersion=domain-v1 weblogic.domainUID=$domainUID weblogic.domainName=$domainName

Choose a reason for hiding this comment

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

Fail the script if the map create or label command fails?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

@@ -0,0 +1,150 @@
# Copyright 2017, 2018, Oracle Corporation and/or its affiliates. All rights reserved.

Choose a reason for hiding this comment

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

Ideally, this script should get all of its values from env vars instead of search-and-replace seds...

Copy link
Member Author

Choose a reason for hiding this comment

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

Not so sure. If I understand the request correctly, we'll expose about the entire domain topology to the create domain job's Pod env.

Choose a reason for hiding this comment

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

I think this is helpful as the script isn't easy to edit if it depends on substitution, since it is a hybrid of both a template and a script. An alternative since this is jython: the script could just read the inputs file directly as a properties file (one line of code I think - the current input file is already in prop file format), and then could get the values as props instead of env vars or sed-substitution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. I'll change the sample script to use env variables then.

do
local outputFile=$fname
sed -i -e "s:%NAMESPACE%:$namespace:g" ${outputFile}
sed -i -e "s:%WEBLOGIC_CREDENTIALS_SECRET_NAME%:${weblogicCredentialsSecretName}:g" ${outputFile}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you match the operator helm chart? It uses 'image', 'imagePullPolicy' and 'imagePullSecrets' (where the secret is the actual list of image pull secrets, instead of just a secret name, which would mean changing the same to parse yaml, instead of simple string values).

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we should if we want to have a consistent/better way of doing the same thing. I'll take a look at the operator helm chart.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am changing the property names to be consistent with what we have in the operator helm charts: weblogicImage -> image, and weblogicImagePullSecretName -> imagePullSecretName. Having discussed with Tom offine, we decided to leave the sample to only support one image pull secret.

sed -i -e "s:%DOMAIN_PVC_NAME%:${persistentVolumeClaimName}:g" ${outputFile}
sed -i -e "s:%DOMAIN_ROOT_DIR%:${domainPVMountPath}:g" ${outputFile}
sed -i -e "s:%DOMAIN_LOGS_DIR%:${domainPVMountPath}/logs:g" ${outputFile}
sed -i -e "s:%CREATE_DOMAIN_SCRIPT_DIR%:${createDomainScriptsMountPath}:g" ${outputFile}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really want different vars for the create domain script directory and name, instead of just a single pathname?

Copy link
Member Author

Choose a reason for hiding this comment

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

There can be multiple shell scripts involved in creating the domain home. We need to know the script that the job needs to run.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

…x copyright

Signed-off-by: doxiao <dongbo.xiao@oracle.com>
… and image pull secrete property names

Signed-off-by: doxiao <dongbo.xiao@oracle.com>
@rjeberhard
Copy link
Member

@doxiao @tbarnes-us @moreaut If comments are addressed, would you please approve the review? Thanks.

@tbarnes-us
Copy link

tbarnes-us commented Oct 2, 2018

@rjeberhard LGTM My comments are about work that should be done, but doesn't need to be in this pull.

Edit: It turns out Dongbo had already addressed the comments. So make that LEGTM

@moreaut
Copy link
Contributor

moreaut commented Oct 2, 2018

The image, imagePullSecretName changes look good to me.

I just noticed that this sample doesn't support imagePullPolicy.
I don't think we need to hold up the merge for this, but, should we add it later?

@doxiao
Copy link
Member Author

doxiao commented Oct 2, 2018

I just noticed that this sample doesn't support imagePullPolicy.
I don't think we need to hold up the merge for this, but, should we add it later?

The original create domain job in 1.x does not support imagePullPolicy. It is hardcoded to IfNotPresent. Note that this is the weblogic 12.2.1.3 image. If it is important to support it in the sample, I can add it (either in this PR or the next one).

@moreaut
Copy link
Contributor

moreaut commented Oct 2, 2018

Hi Dongbo, I think we should add imagePullPolicy eventually - I'm OK if you add it in this PR or in a subsequent one.

@doxiao
Copy link
Member Author

doxiao commented Oct 2, 2018

Hi Dongbo, I think we should add imagePullPolicy eventually - I'm OK if you add it in this PR or in a subsequent one.

Okay. I'll add it in this PR.

doxiao added 2 commits October 2, 2018 10:19
Signed-off-by: doxiao <dongbo.xiao@oracle.com>
Signed-off-by: doxiao <dongbo.xiao@oracle.com>
@rjeberhard rjeberhard merged commit aa71c37 into develop Oct 4, 2018
@rjeberhard rjeberhard deleted the owls68835_add_customization branch December 15, 2018 19:49
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.

4 participants