Skip to content
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

DS8K support : change ibm-ubiquity-db to configurable #186

Merged
merged 3 commits into from
Mar 1, 2018

Conversation

hfeish
Copy link
Contributor

@hfeish hfeish commented Feb 7, 2018

get ibm ubiquity db name from ubiquity Pod env setting.

k8s side of thing -> IBM/ubiquity-k8s#167

Signed-off-by: feihuang feihuang@feihuangs-mbp.cn.ibm.com


This change is Reviewable

Signed-off-by: feihuang <feihuang@feihuangs-mbp.cn.ibm.com>
@coveralls
Copy link

coveralls commented Feb 7, 2018

Coverage Status

Coverage decreased (-0.07%) to 54.731% when pulling fe2a3be on fix/UB-945_Ubiquity_For_DS8k_P3 into c94b05f on dev.

@shay-berman
Copy link
Contributor

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


database/volume.go, line 26 at r1 (raw file):

var VolumeNameSuffix = os.Getenv(keyPsqlDbPVName)
//const VolumeNameSuffix = "ibm-ubiquity-db"

You should apply here a critical fix:
We need to set a default value in case KeyPsqlDBPVname is not defined, in order to support customer that didn't upgrade the yml file.
So please if env is empty, then set it to VolumeNameSuffix. So uncomment the const and use it as default

In addition make sure we don't have other ibm-ubiquity-db in the code that we forgot to change.

thanks


Comments from Reviewable

Signed-off-by: feihuang <feihuang@feihuangs-mbp.cn.ibm.com>
@shay-berman
Copy link
Contributor

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.


database/volume.go, line 26 at r1 (raw file):

Previously, shay-berman wrote…

You should apply here a critical fix:
We need to set a default value in case KeyPsqlDBPVname is not defined, in order to support customer that didn't upgrade the yml file.
So please if env is empty, then set it to VolumeNameSuffix. So uncomment the const and use it as default

In addition make sure we don't have other ibm-ubiquity-db in the code that we forgot to change.

thanks

ok good I saw your fix for that.
now its ok. thanks


database/volume.go, line 36 at r2 (raw file):

}

func getDBVolumeName(dbPVNameENVKey string) string {

If posible move this function to utils, and call it getEnv(envName, defaultValue) and make it generic.


Comments from Reviewable

Signed-off-by: feihuang <feihuang@feihuangs-mbp.cn.ibm.com>
@hfeish
Copy link
Contributor Author

hfeish commented Feb 28, 2018

@shay-berman move func to get dbname from ENV to IBM/ubiquity/utils/utils

@hfeish
Copy link
Contributor Author

hfeish commented Mar 1, 2018

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


database/volume.go, line 36 at r2 (raw file):

Previously, shay-berman wrote…

If posible move this function to utils, and call it getEnv(envName, defaultValue) and make it generic.

move this function to Ubiquity/Utils/utils, and make it generic


Comments from Reviewable

@shay-berman
Copy link
Contributor

:lgtm:


Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@hfeish hfeish merged commit 1efe4b1 into dev Mar 1, 2018
@shay-berman shay-berman changed the title change ibm-ubiquity-db to configurable DS8K support : change ibm-ubiquity-db to configurable Mar 12, 2018
@shay-berman shay-berman deleted the fix/UB-945_Ubiquity_For_DS8k_P3 branch March 25, 2018 11:10
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.

None yet

3 participants