-
Notifications
You must be signed in to change notification settings - Fork 83
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
Fixes #27374 - Add export-default to export default cv like CDN #666
Conversation
Issues: #27374 |
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 code looks fine to me (I didn't test it). Commented inline with an idea for how to test.
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.
Looks good. Thanks @chris1984! One other comment/question for you.
@akofink updated based on the last comment, also added another test to make it fail and check for that: Here are the results of the test from my devel box: Good:
[vagrant@centos7-katello-devel ~]$ hammer content-view version export-default --export-dir /home/vagrant/test
Default content view export is available at /home/vagrant/test
[vagrant@centos7-katello-devel ~]$ ll /home/vagrant/test/
total 4
drwxr-x---. 4 vagrant apache 57 Jul 23 18:43 Default_Organization
-rw-r--r--. 1 vagrant apache 20 Jul 23 18:43 listing
Fail:
[vagrant@centos7-katello-devel ~]$ hammer content-view version export-default --export-dir /root
rsync: change_dir#1 "/root" failed: Permission denied (13)
rsync error: errors selecting input/output files, dirs (code 3) at main.c(614) [Receiver=3.1.2]
Could not export the content view at /root Mike said he won't be able to test till next week, so up to you if you want to merge/ack or let it sit. I am fine with either since it is not a urgent issue. I had to add the puts statement since it is not giving the user good/fail messages prob because of the |
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.
I'm fine with merging. The code is fairly straightforward. I have some comments about the puts statements :)
@akofink updated. I removed the puts and replaced with the output method, removed the failure/success messages since they are not used and also removed the return statements. Here are the updated tests: Good:
[vagrant@centos7-katello-devel hammer_cli_katello-0.18.0]$ hammer content-view version export-default --export-dir /home/vagrant/test
Default content view export is available at /home/vagrant/test
[vagrant@centos7-katello-devel hammer_cli_katello-0.18.0]$ ll /home/vagrant/test
total 4
drwxr-x---. 4 vagrant apache 57 Jul 23 18:43 Default_Organization
-rw-r--r--. 1 vagrant apache 20 Jul 23 18:43 listing
Bad:
[vagrant@centos7-katello-devel hammer_cli_katello-0.18.0]$ hammer content-view version export-default --export-dir /root
rsync: change_dir#1 "/root" failed: Permission denied (13)
rsync error: errors selecting input/output files, dirs (code 3) at main.c(614) [Receiver=3.1.2]
Could not export the default content view at /root |
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.
Thanks @chris1984! ACK!
No description provided.