-
Notifications
You must be signed in to change notification settings - Fork 112
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 #37733 - Add audits of ansible role changes on hosts #729
Conversation
2c0c935
to
5252504
Compare
@adamlazik1 Could you add tests to this PR to help validate the changes? |
52bf11f
to
1117f72
Compare
Added the test, I hope it's in the correct place. |
test/unit/host_ansible_role_test.rb
Outdated
@@ -17,4 +17,25 @@ class HostAnsibleRoleTest < ActiveSupport::TestCase | |||
end | |||
should validate_uniqueness_of(:ansible_role_id).scoped_to(:host_id) | |||
end | |||
|
|||
describe 'auditing' do | |||
test 'audit creation and deletion of host ansible roles' do |
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 recommend splitting this into separate tests for each action. One test should focus on verifying the audit record for creation, and another for deletion.
test/unit/host_ansible_role_test.rb
Outdated
ansible_role = FactoryBot.create(:ansible_role) | ||
host_ansible_role = FactoryBot.create(:host_ansible_role, :with_auditing, host_id: host.id, ansible_role_id: ansible_role.id) | ||
|
||
assert_not host_ansible_role.audits.blank? |
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.
IMO, it would be clearer to ensure that each test verifies only one audit record is created. This helps confirm that the auditing is working as expected.
1117f72
to
4ac7663
Compare
Alright, I split the test and tried to extract some common parts. |
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.
Could you please update the assert_equal
statements so that the expected value is the first argument, in line with common practice?
This records audits when ansible role is added to a host or removed from a host. Defining the `to_label` method is necessary so the name of host ansible role is shown in the format `role / host` rather than `id / host`.
4ac7663
to
e79f2a8
Compare
Done. |
This records audits when ansible role is added to a host or removed from a host.
Defining the
to_label
method is necessary so the name of host ansiblerole is shown in the format
role / host
rather thanid / host
.