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
[RHBA-577] Create OpenShift templates based on RHBA LA release -- 1st draft #9
[RHBA-577] Create OpenShift templates based on RHBA LA release -- 1st draft #9
Conversation
from: "[a-zA-Z]{6}[0-9]{1}!" | ||
generate: expression | ||
required: false | ||
- displayName: Smart router controller token |
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 display name do not look correct. Shouln't it be Kie Server Controller token?
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.
fixed the copy & paste error
selector: | ||
deploymentConfig: "${APPLICATION_NAME}-rhbacentrmon" | ||
metadata: | ||
name: "${APPLICATION_NAME}-rhbacentrmonmon" |
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.
At this line there is a typo "monmon"
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.
fixed
name: IMAGE_STREAM_NAMESPACE | ||
value: openshift | ||
required: true | ||
- displayName: MySQL Image Stream Tag |
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 template is only for Business Central and there is no need for a DB so all environment variables for DB can be removed.
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.
fixed by taking out the DB section for Business Central
annotations: | ||
description: The Business Central web server's https port. | ||
|
||
|
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.
empty lines can be removed
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.
removed empty lines
containerPort: 8443 | ||
protocol: TCP | ||
env: | ||
- name: KIE_ADMIN_PWD |
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 KIE_ADMIN_PWD and KIE_ADMIN_USER should use the same values as the MAVEN_REPO_PASSWORD and MAVEN_REPO_USERNAME respectively or else they will have different generated values
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.
Fixed in rhba70-full-mysql.yaml and rhba70-full-mysql-persistent.yaml
- name: MAVEN_REPO_USERNAME
value: "${KIE_ADMIN_USER}"
- name: MAVEN_REPO_PASSWORD
value: "${KIE_ADMIN_PWD}"
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 configuration is not valid for case, when is connected an external maven repository. For external maven repository can be set different user and password.
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.
@jakubschwan then the maven admin shouldn't have a default and the password not be generated. The original approach prevents the template from working. The description already provides enough information for the user.
@calvinzhuca given @jakubschwan's point you could leave the old values and remove the default value for the admin and password generation, leaving up to the template user to decide which values to provide
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.
ok, updated based on you two's feedback
JIRA is at https://issues.jboss.org/browse/RHBA-577 |
deploymentConfig: "${APPLICATION_NAME}-rhbacentr" | ||
application: "${APPLICATION_NAME}" | ||
spec: | ||
serviceAccountName: businesscentral-service-account |
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 service account should be also created in the template and removed from the description. If it already exists the template will continue processing the other objects. The same for all the other service accounts in any template
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.
Please squash these 7 commits into 1.
@@ -280,7 +280,6 @@ objects: | |||
application: "${APPLICATION_NAME}" | |||
annotations: | |||
description: The Business Central web server's http port. | |||
service.alpha.openshift.io/dependencies: '[{"name": "${APPLICATION_NAME}-mysql", "kind": "Service"}]' |
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.
Curious why the removal of the dependencies (here and 3 other places)? Don't we want to make sure the mysql service is available before others that depend on it?
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.
@errantepiphany this was removed from the console which was its only purpose. See openshift/origin-web-console#2319
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.
ok, I squashed the 7 commits
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.
@ruromero Okay, thanks. I thought it was an actual service dependency (not just a console UI thing), but after consulting a couple other people, that doesn't seem to be the case.
Signed-off-by: calvin zhu <calvinzhu@hotmail.com>
Thanks for submitting your Pull Request!
https://issues.jboss.org/browse/RHBA-577
Please make sure your PR meets the following requirements:
[CLOUD-XYA] Subject
CONTRIBUTING.md
)Signed-off-by: Your Name <yourname@example.com>
- usegit commit -s