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

Fixes #27346 - show audit records for host create #6969

Merged
merged 2 commits into from Sep 4, 2019

Conversation

kgaikwad
Copy link
Member

@kgaikwad kgaikwad commented Aug 9, 2019

adding @ares, @tbrisker in loop.

Instead of adding hard-coded list into main objects, I have considered parent classes which are not abstract and having DB table.

@theforeman-bot
Copy link
Member

Issues: #27346

@tbrisker
Copy link
Member

Thanks @kgaikwad! I think this may be an outcome of having multiple different ways we do auditing currently around STI. perhaps we should try to think how we can make it simpler in a way that works for everything? Some differences off the top of my head:

  1. Host::* audits are manually converted to Host::Base in AuditExtensions before saving - i.e. audited on subclass but saved as parent class.
  2. Taxonomy, LookupKey, Nic::* are saved as Location/Organization, PuppetClassLK/VariableLK and Nic type respectively, i.e. audited on parent class but saved with subclass. (also from AuditExtensions)
  3. Parameter, Template are audited on the subclass
  4. OperatingSystem, Setting and many others are audited on the parent class

I wonder if simplifying these options (maybe just using the last two - so the audit definition is on the class we care about) would be possible and make some of the rest of the work around audits easier and less error prone?

@kgaikwad
Copy link
Member Author

Thank you @tbrisker for bringing this.
I am trying to understand reason behind this implementation (1) & (2). Please correct me If I am getting it wrongly but these changes were added to avoid complications while showing audits as well as to minimize dataset. But I will check more on these to find a better solution.

@ares
Copy link
Member

ares commented Sep 3, 2019

@tbrisker is this acceptable at least as an interim solution? While I'd like to investigate if unification of STI classes auditing make sense, I'd also like to get this fixed.

Quick comment on your findings, points 3 and 4 makes sense to me from user perspective. Point 1 seems reasonable. Point 2 - I can understand taxnomy and lookup keys, not sure about nics though.

@tbrisker
Copy link
Member

tbrisker commented Sep 3, 2019

@ares my comment was more of an observation and thinking out loud than a requirement. If @kgaikwad found a simple way to fix part of it in this PR it would be good, but if not we can stay with this solution, or do an explicit workaround for the hosts so we know to remove it once we fix the sti handling.
Can we add a test though to make sure this doesn't break again?

@ares
Copy link
Member

ares commented Sep 4, 2019

ok, thanks @tbrisker, @kgaikwad would you mind adding a test for host object? then I think we're ready to merge

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

test

@kgaikwad
Copy link
Member Author

kgaikwad commented Sep 4, 2019

@ares, @tbrisker,
Sorry for the delay.
I have added test which will check 'Host::Base' included in audit's main_objects.

@tbrisker, My apologies. I will need some more time to investigate changes around an unification of STI classes.

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

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

Thank you both @kgaikwad and @tbrisker, I'm merging as it fixes the issue and is a good solution until we simplify the STI in audits.

@ares ares merged commit 5b47a90 into theforeman:develop Sep 4, 2019
Ron-Lavi pushed a commit to Ron-Lavi/foreman that referenced this pull request Jul 5, 2020
Fixes #27346 - show audit records for host create (theforeman#6969)

(cherry picked from commit 5b47a90)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants