-
Notifications
You must be signed in to change notification settings - Fork 216
Convert MII sample to use aux images... #3751
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
Conversation
…on-aux instructions in the WL images doc, plus some other aux image related doc/explain tweaks and fixes.
rosemarymarano
left a comment
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.
Edits and suggestions.
documentation/4.0/content/samples/domains/model-in-image/initial.md
Outdated
Show resolved
Hide resolved
documentation/4.0/content/samples/domains/model-in-image/update3.md
Outdated
Show resolved
Hide resolved
documentation/4.0/content/samples/domains/model-in-image/update3.md
Outdated
Show resolved
Hide resolved
|
Kudos, SonarCloud Quality Gate passed!
|
ankedia
left a comment
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.
LGTM. I have minor comments and a question on whether we should change the name of auxiliary images created in the samples to something simpler (e.g. model-in-image-ai:WLS-v1 or model-in-image:WLS-v1) and have a different name for the image with model files layered on the WebLogic image.
|
|
||
| {{% notice note %}} | ||
| If you set `modelHome` and `wdtInstallHome` to a non-default value, | ||
| then the operator will ignore WDT model and installation files |
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.
Should this say "domain" instead of the operator and "in Auxiliary images" similar to line 116-117 in documentation/4.0/content/managing-domains/model-in-image/auxiliary-images.md ?
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.
Probably. That's the original wording copied from before these changes. I changed it to domain in other locations IIRC.
Since the pull is already merged, I will create a new pull.
|
|
||
| {{% notice note %}} | ||
| If you set `modelHome` and `wdtInstallHome` to a non-default value, | ||
| then the operator will ignore WDT model and installation files |
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.
Should this say "domain" instead of the operator similar to line 116 in documentation/4.0/content/managing-domains/model-in-image/auxiliary-images.md?
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.
Probably. That's the original wording copied from before these changes. I changed it to domain in other locations IIRC.
Since the pull is already merged, I will create a new pull.
|
|
||
| - Image `model-in-image:WLS-v1` with: | ||
| - A WebLogic installation | ||
| - Auxiliary image `model-in-image:WLS-AI-v1` with: |
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.
Since we are changing the default to use auxiliary images, I wonder if we should change the MII image name/tag to something like model-in-image-ai:WLS-v1 or just model-in-image:WLS-v1 and then have a different tag for the non-auxiliary image models.
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 sample directory structure, internal sample helper scripts, and testing solely rely on the ":" suffix throughout to distinguish images, so changing the predicate is impractical.
As for changing the suffix, how about we think about changing the current "WLS-v1" to "WLS-LI-v1", where LI indicates 'Layered Image' as opposed to 'Auxiliary Image'? We could then leave "WLS-AI-v1" unchanged. Plus this is more clear IMO, as then all tags would rigorously have a second component of either 'LI' or 'AI' that is meaningful rather than relying on knowing that a shorter tag indicates something important...
Note that this is a change that's not directly in the scope of this merge as the nomenclature was chosen well before 4.0 (plus the merge already occurred).
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.
Thoughts? @rjeberhard ?
* Convert MII sample instructions to use aux images, supply 'old way' non-aux instructions in the WL images doc, plus some other aux image related doc/explain tweaks and fixes.
* Convert MII sample instructions to use aux images, supply 'old way' non-aux instructions in the WL images doc, plus some other aux image related doc/explain tweaks and fixes.








...plus:
kubectl explainfor modelHome and wdtInstallHome.