[bulder] Add MsSQL and OracleXE #533
Conversation
| new ServiceAccountBuilder().withNewMetadata().withName(ANYUID_SERVICE_ACCOUNT).endMetadata().build()); | ||
| } | ||
| } | ||
| try (OpenShift admin = OpenShifts.admin()) { |
There was a problem hiding this comment.
This is common to both MsSQL and OracleXE. Have you tried to avoid using admin ? It can happen we will be forced to run this code on cluster without admin. Would that be possible? If not can you document which permission regular master user will be missing?
There was a problem hiding this comment.
Regular master is missing permissions to add security context constraint. I added this info in the comment before the classes.
OndrejGerzicak
force-pushed
the
feature/addDbsToAppbuilder
branch
from
b440edd
to
37331d6
Compare
|
|
||
| //Setting up the anyuid service account | ||
| try (OpenShift master = OpenShifts.master()) { | ||
| if (master.getServiceAccount("ANYUID_SERVICE_ACCOUNT") == null) { |
There was a problem hiding this comment.
this needs to be unquoted to actually use the variable's value
OndrejGerzicak
force-pushed
the
feature/addDbsToAppbuilder
branch
from
37331d6
to
22a9c1e
Compare
| super.configureContainer(containerBuilder); | ||
|
|
||
| //Setting up the anyuid service account | ||
| //Don't close the OpenShift Client instance, it could cause trouble with pod shell access |
There was a problem hiding this comment.
Not sure we want this info here, it is a general note more suited for README or maybe OpenShifts.master() javadoc .. but it isn't a socpe of this PR at all
There was a problem hiding this comment.
I added this comment just in case someone else would see this line and wanted to use the try with resource block as it's suggested by IDEA.
Agree that it should be mentioned elsewhere though.
There was a problem hiding this comment.
If IDEA is suggesting something we know can bring problem, can it be fixed in XTF code? Can we handle it as XTF bug? To change XTF API in backward compatible manner so IDEA stop suggesting try with resource?
There was a problem hiding this comment.
you can skip the IDEA from suggesting this by introducing comment
| //Don't close the OpenShift Client instance, it could cause trouble with pod shell access | |
| //noinspection resource |
But IDEA is not the only IDE in the world so I don't know if we want to go this road
OndrejGerzicak
force-pushed
the
feature/addDbsToAppbuilder
branch
from
1c7f9b0
to
4e9b581
Compare
|
@mnovak1 please work with @OndrejGerzicak (@jbliznak) to merge this and release this for their work. |
OndrejGerzicak
force-pushed
the
feature/addDbsToAppbuilder
branch
from
4e9b581
to
edad799
Compare
|
LGTM. merging. Thanks! |
This PR adds OracleXe and MsSQL Server databases to the builder. Since the provided images don't create custom databases and users on setup, we need to use the default, that's why I added the new constructors to AbstractDatabase and AbstractSQLDatabase classes.
Because the defualt database of OracleXE is
XEI've come across a possible issue with Uppercase dbName that gets to the opensihft deployment config name. Although the OracleXE database server is case insensitive, it is possible to crate a case sensitive SQL server so it's seems safer to me to use lower case equivalent in the deployment name deployment config namePlease make sure your PR meets the following requirements: