-
Notifications
You must be signed in to change notification settings - Fork 5
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
SECURESIGN-55 | Monitoring of Sigstore containers with Cockpit #112
Conversation
64d0422
to
7f53c05
Compare
@bkabrda When you get a chance can you take a look at this?, To me, it didn't make sense to add 'redhat.rhel_system_roles' as a dependency since cockpit is optional wdyt?, and please correct me if I am wrong |
6506bf6
to
949be35
Compare
5463656
to
a1412f4
Compare
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.
Hi Jason, nice job! I provided some inline comments to address. I think it's good to not depend on the rhel system role if we don't require it by default. However, I think in this case the README should list a minimum version of that collection that we guarantee will work.
README.md
Outdated
sudo usermod -aG wheel cockpit-user | ||
``` | ||
|
||
Note: You may need to log out and then log back in to the Cockpit interface for these changes to take effect. |
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.
It's nice to have these commands, but how about we provide them as a set of tasks that are run optionally, so that user can just set a variable to true
and get this set up by the role?
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.
Sure ofc, never even thought of doing that but now that you mention that makes sense :)
9f4c976
to
3ae3b66
Compare
3ae3b66
to
aec7e52
Compare
e290588
to
4b7907e
Compare
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.
LGTM now, thanks for putting all the work in!
No description provided.