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

Collapse keyring_implementation_base and keyring_facard #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tserong
Copy link

@tserong tserong commented Sep 16, 2016

This is admittedly lacking tests, but should be functionally
identical to the previous version.

Signed-off-by: Tim Serong tserong@suse.com

This is admittedly lacking tests, but should be functionally
identical to the previous version.

Signed-off-by: Tim Serong <tserong@suse.com>
@oms4suse
Copy link
Owner

I really like the patch. I want it in.

Since methods, starting with '_' should be private, I totally understand not checking for these, Sadly in the rush to complete work, I by passed this convention, and did some bad behaviour.

This can be seen with:

grep -n  _get_path_keyring_ */*.py

The output showing these issues.

ceph_cfg/mds.py:78:        path_bootstrap_keyring = keyring._get_path_keyring_mds(self.model.cluster_name)
ceph_cfg/mon.py:240:        path_admin_keyring = keyring._get_path_keyring_admin(self.model.cluster_name)
ceph_cfg/mon.py:241:        keyring_path_mon = keyring._get_path_keyring_mon(self.model.cluster_name)
ceph_cfg/osd.py:223:        bootstrap_path_osd = keyring._get_path_keyring_osd(cluster_name)
ceph_cfg/rgw.py:98:        path_bootstrap_keyring = keyring._get_path_keyring_rgw(self.model.cluster_name)

They should all be trivial to fix, but at this moment, I dont have time at the weekend to resolve these after accepting the patch. Any chance you could resolve theses issues in the patch?

@oms4suse
Copy link
Owner

This patch is dependent on #38 being merged first.

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.

2 participants