linux-system-roles / cockpit

https://linux-system-roles.github.io/cockpit/
GNU General Public License v3.0
15 stars 19 forks source link

Use the firewall role and the selinux role from the cockpit role #76

Closed nhosoi closed 1 year ago

nhosoi commented 1 year ago
nhosoi commented 1 year ago

[citest]

nhosoi commented 1 year ago

I forced to push a commit to update README.md after I set [citest]. Unfortunately, it prevents to display the CI test results in the CI test result section... But thanks to check_jenkins.py implemented by @richm . We could see all the tests successfully passed in the output.

TaskID   Started At          Role            PR  Platform               Duration Queue Time Status    
17297    2022-09-28T15:15:37 cockpit         76  RHEL-9.2/ansible-2.13  1713     14         PASSED    
17296    2022-09-28T15:15:37 cockpit         76  RHEL-9.2/ansible-2.9   1683     14         PASSED    
17295    2022-09-28T15:15:35 cockpit         76  RHEL-8.8/ansible-2.13  1779     13         PASSED    
17294    2022-09-28T15:15:35 cockpit         76  RHEL-8.8/ansible-2.9   1819     13         PASSED    
17293    2022-09-28T15:15:32 cockpit         76  RHEL-7.9/ansible-2.13  1817     10         PASSED    
17292    2022-09-28T15:15:32 cockpit         76  RHEL-7.9/ansible-2.9   1769     10         PASSED    
17291    2022-09-28T15:15:30 cockpit         76  CentOS-8/ansible-2.13  2714     7          PASSED    
17290    2022-09-28T15:15:30 cockpit         76  CentOS-8/ansible-2.9   2732     7          PASSED    
17289    2022-09-28T15:15:27 cockpit         76  CentOS-7/ansible-2.13  4243     5          PASSED    
17288    2022-09-28T15:15:27 cockpit         76  CentOS-7/ansible-2.9   4226     10         PASSED    
17287    2022-09-28T15:15:22 cockpit         76  Fedora-36/ansible-2.13 3120     5          PASSED    
17286    2022-09-28T15:15:22 cockpit         76  Fedora-36/ansible-2.9  3143     5          PASSED    
17285    2022-09-28T15:15:17 cockpit         76  Fedora-35/ansible-2.13 3128     0          PASSED    
17284    2022-09-28T15:15:17 cockpit         76  Fedora-35/ansible-2.9  3081     0          PASSED    
17283    2022-09-28T15:15:12 cockpit         76  RHEL-9.2/ansible-2.13  1713     19         PASSED    
17282    2022-09-28T15:15:12 cockpit         76  RHEL-9.2/ansible-2.9   1806     19         PASSED    
17281    2022-09-28T15:15:07 cockpit         76  RHEL-8.8/ansible-2.13  1845     15         PASSED    
17280    2022-09-28T15:15:07 cockpit         76  RHEL-8.8/ansible-2.9   1794     15         PASSED    
17279    2022-09-28T15:15:02 cockpit         76  RHEL-7.9/ansible-2.13  1809     10         PASSED    
17278    2022-09-28T15:15:02 cockpit         76  RHEL-7.9/ansible-2.9   1801     10         PASSED    
17277    2022-09-28T15:14:59 cockpit         76  CentOS-8/ansible-2.13  2738     7          PASSED    
17276    2022-09-28T15:14:59 cockpit         76  CentOS-8/ansible-2.9   2707     7          PASSED    
17275    2022-09-28T15:14:57 cockpit         76  CentOS-7/ansible-2.13  4292     10         PASSED    
17274    2022-09-28T15:14:57 cockpit         76  CentOS-7/ansible-2.9   4396     10         PASSED    
17273    2022-09-28T15:14:52 cockpit         76  Fedora-36/ansible-2.13 3191     5          PASSED    
17272    2022-09-28T15:14:52 cockpit         76  Fedora-36/ansible-2.9  3150     5          PASSED    
17271    2022-09-28T15:14:47 cockpit         76  Fedora-35/ansible-2.13 3179     0          PASSED    
17270    2022-09-28T15:14:47 cockpit         76  Fedora-35/ansible-2.9  3142     0          PASSED    
richm commented 1 year ago

hmm - also looks like we need to change the tests so that you can run multiple tests at the same time on the same controller:

fatal: destination path '/WORKDIR/dist-git-cockpit-use_roles-hJbYsU/tests/roles/linux-system-roles.certificate' already exists and is not an empty directory.

but this is unrelated to this PR - we had to change some of the other roles to do something similar e.g. https://github.com/linux-system-roles/nbde_client/pull/80 so that each test gets its own temp directory

nhosoi commented 1 year ago

preciate if you could split this up in to separate commits (one for public, one for firewalld, one for selinux). Cheers!

How about the test script, tests/tasks/check_firewall_selinux.yml? Do you want to make it into two files; one for firewall and another for selinux?

And should README.md needs to be separated, as well? Some parts are a bit difficult to split clearly, especially the Requirements section...

Let me split this pr into 1) firewall, 2) selinux, 3) README, and 4) tests. Hope it's ok. @martinpitt, ^^^^^^

martinpitt commented 1 year ago

@nhosoi : a new feature and a test should always be in the same commit, as they belong together. Same for the documentation. The point is not to make commits as small as possible, but only do one thing/feature/bug fix/change per commit (including the corresponding docs, tests, etc.). That makes them easier to understand/review, possible to bisect, and backport.

I leave it up to you whether you want to split tests/tasks/check_firewall_selinux.yml; IMHO the functionality belongs together, and I'd even rename it to check_port.yml. Then the "firewall" commit would do the firewalld check there, and the "selinux" commit would add the web_sm_t check to the same file.

nhosoi commented 1 year ago

[citest]

nhosi commented 1 year ago

Thanks @nhosi! Test-wise you got a lot further than me, I'm happy for you to take over, and I'll close my PR #73. I have several small comments, and I'd really appreciate if you could split this up in to separate commits (one for public, one for firewalld, one for selinux). Cheers!

Hi! You tagged a wrong GitHub account. I will unsubscribe this chat. Br, @nhosi

nhosoi commented 1 year ago

[citest]

nhosoi commented 1 year ago

[citest]

nhosoi commented 1 year ago

[citest]

nhosoi commented 1 year ago

[citest]

richm commented 1 year ago

ok - looks good - please rebase on top of the latest master branch and we can merge

nhosoi commented 1 year ago

ok - looks good - please rebase on top of the latest master branch and we can merge

Thank you, @richm. Done!

richm commented 1 year ago

spoke too soon - looks like merge is blocked - @martinpitt needs to review, and clear the requested changes flag.