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

Add target mount for gluster block #7198

Conversation

mjudeikis
Copy link
Contributor

@mjudeikis mjudeikis commented Feb 19, 2018

Adding target folder mount for gluster block to persist configuration.
Resolves: #7198
BZ: 1540080

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 19, 2018
@mjudeikis
Copy link
Contributor Author

mjudeikis commented Feb 19, 2018

@datarace, is this the fix you require?

cc: @jarrpa

@datarace
Copy link

LGTM

@mjudeikis
Copy link
Contributor Author

/retest

@SaravanaStorageNetwork
Copy link
Contributor

minor comment:

/s/glusterfs-target/glusterfs-block/

@mjudeikis
Copy link
Contributor Author

mjudeikis commented Feb 19, 2018

This is my question too...

Should we be following host path naming convention (as it is now) or should we do purpose based (as per suggestion).

I prefer host path naming convention for HostPath and Purpose for config/secrets/volumes as it makes debugging much easier when tracking volume names.

@jarrpa
Copy link
Contributor

jarrpa commented Feb 19, 2018

This is backporting an existing feature in the templates of cns-deploy, where we named it gluster-block. Regardless, this is good enough for now. It introduces some undesirable divergence between the two template locations, but that's just more incentive to consolidate them into one faster. :)

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2018
@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@jarrpa
Copy link
Contributor

jarrpa commented Feb 19, 2018

@mjudeikis Can you reference "Resolves: #7198" and any relevant BZs in the initial comment / description?

@mjudeikis
Copy link
Contributor Author

/retest

@mjudeikis
Copy link
Contributor Author

@jarrpa done. +1 for consolidation plan ;)

@jarrpa
Copy link
Contributor

jarrpa commented Feb 19, 2018

/retest

1 similar comment
@mjudeikis
Copy link
Contributor Author

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 05e6e2c into openshift:master Feb 19, 2018
mjudeikis added a commit to mjudeikis/openshift-ansible that referenced this pull request Feb 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants