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 initial apparmor profiles #716

Merged
merged 4 commits into from
Jul 30, 2018
Merged

Add initial apparmor profiles #716

merged 4 commits into from
Jul 30, 2018

Conversation

isuzdal
Copy link
Contributor

@isuzdal isuzdal commented Jul 13, 2018

This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Jul 13, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1.
Reviewable status: 0 of 2 LGTMs obtained

Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 LGTMs obtained, and 1 stale

Copy link
Contributor

@jellonek jellonek left a comment

Choose a reason for hiding this comment

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

That would be the first and the only .rst file in our repo, without any specific reason. Could you translate it into markdown?

Also could you add some small section to https://github.com/Mirantis/virtlet/blob/master/docs/architecture.md or/and to main README.md about that?

Also take a look on https://github.com/Mirantis/virtlet/blob/master/deploy/real-cluster.md#installing-virtlet-on-a-real-cluster where we have an info that app armor needs to be disabled. It could be pointed that using these files it could be made working with Virtlet.

The last thing would be a position of this added directory. Could you move it into e.g. deploy/ ?

Reviewed 4 of 4 files at r1.
Reviewable status: 1 of 2 LGTMs obtained


apparmor/README.rst, line 5 at r1 (raw file):

======================

In order to get virtlet works on an apparmor enabled environment

I'm not fully confident about that, but maybe In order do get virtlet working in an apparmor enabled environment would be more grammatically correct?

@isuzdal
Copy link
Contributor Author

isuzdal commented Jul 17, 2018

@jellonek Sure will do. But I'm in doubt what actually should I describe in the architecture. Because the apparmor is not a part of the virtlet. IMO it's better to mention the apparmor in https://github.com/Mirantis/virtlet/blob/master/deploy/real-cluster.md
What do you think?

@jellonek
Copy link
Contributor

It's probably the only place where we have an info about apparmor disabling, so yes, imo it's best place.

Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained, and 1 stale


deploy/apparmor/README.md, line 6 at r2 (raw file):

an [apparmor](https://gitlab.com/apparmor/apparmor/wikis/home) enabled environment follow the next steps:

* install these profiles into the corresponding directory (/etc/apparmor.d/ if you use Debian or its derivatives)

would be nice to mention where are the profiles before this phrase ("these" profiles). E.g. "the profiles located in this directory"

also, add cmd for installing the profiles
@jellonek
Copy link
Contributor

LGTM

Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 6 files at r2, 1 of 1 files at r3.
Reviewable status: 0 of 2 LGTMs obtained, and 1 stale

Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 LGTMs obtained, and 1 stale

Copy link
Contributor

@pigmej pigmej left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 6 files at r2, 1 of 1 files at r3.
Reviewable status: 1 of 2 LGTMs obtained, and 1 stale

@pigmej pigmej merged commit 502994a into Mirantis:master Jul 30, 2018
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.

5 participants