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

[DNM] [SES5] qa: add --apparmor option to all test scripts #1031

Closed
wants to merge 5 commits into from

Conversation

smithfarm
Copy link
Contributor

$SUBJ is self-explanatory

@smithfarm smithfarm added the QA label Mar 23, 2018
@smithfarm smithfarm requested a review from jan--f March 23, 2018 15:34
@smithfarm smithfarm changed the title qa: add --apparmor option to all test scripts [SES5] qa: add --apparmor option to all test scripts Mar 23, 2018
Copy link
Contributor

@jan--f jan--f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other then inline comment, looks good to me.

CLI=""
SSL=""
while true ; do
case "$1" in
--cli) CLI="cli" ; shift ;;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deletion seems to be an accident?

Signed-off-by: Nathan Cutler <ncutler@suse.com>
Signed-off-by: Nathan Cutler <ncutler@suse.com>
Signed-off-by: Nathan Cutler <ncutler@suse.com>
Signed-off-by: Nathan Cutler <ncutler@suse.com>
@@ -430,6 +430,7 @@ EOF
function ceph_apparmor {
salt '*' state.apply ceph.apparmor
salt '*' cmd.run 'systemctl | grep -i apparmor'
salt '*' cmd.run '/usr/sbin/apparmor_status'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imho it would be better to use /usr/sbin/aa-status here. /usr/sbin/apparmor_status is a symlink to aa-status...might be suse specific..?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So use aa-status here, too?

contents = self.local.cmd(self.search , 'cmd.shell', [ '/usr/sbin/apparmor_status --enabled 2>/dev/null; echo $?' ], expr_form="compound")

Copy link
Contributor

@jan--f jan--f Mar 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah thats probably a good idea as well.

edit: I'm on it.

@smithfarm smithfarm added the DNM label Mar 27, 2018
@smithfarm smithfarm changed the title [SES5] qa: add --apparmor option to all test scripts [DNM] [SES5] qa: add --apparmor option to all test scripts Mar 27, 2018
apparmor_status is just a symlink to aa-status, so use the latter.

Signed-off-by: Nathan Cutler <ncutler@suse.com>
@swiftgist
Copy link
Contributor

This is still set to DNM. Also, I thought we had a smoketest for apparmor on the horizon.

@jschmid1
Copy link
Contributor

we should close that. any objections @smithfarm ?

@smithfarm
Copy link
Contributor Author

closing in favor of apparmor coverage in ceph.smoketests orchestration

@smithfarm smithfarm closed this Jul 25, 2018
@smithfarm smithfarm deleted the wip-qa-apparmor branch July 25, 2018 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants