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.

Sign up for GitHub

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

Jump to bottom

Fixes #27075 - fact names check #267

Merged
merged 1 commit into from
Nov 21, 2019
Merged

Fixes #27075 - fact names check #267

merged 1 commit into from
Nov 21, 2019

Conversation

lzap
Copy link
Member

@lzap lzap commented Jun 18, 2019

SSIA please test in real environment with real facts :-)

@theforeman-bot
Copy link
Member

Issues: #27075

@lzap
Copy link
Member Author

lzap commented Jun 20, 2019

@mccun934 hey have you got to try it? Any comments? I haven't tried this on instances with some real data, so not sure how is the experience (how fast this is). Shall we aim for 10k default threshold?

kgaikwad
kgaikwad suggested changes Jun 21, 2019
View reviewed changes
Copy link
Member

@kgaikwad kgaikwad 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 @lzap for submitting pull-request.

I have added few inline comments.

definitions/checks/foreman_facts_names.rb Outdated Show resolved Hide resolved
definitions/checks/foreman_facts_names.rb Outdated Show resolved Hide resolved
definitions/checks/foreman_facts_names.rb Outdated Show resolved Hide resolved
@lzap
Copy link
Member Author

lzap commented Jul 22, 2019

How about this? Full disclosure - untested :-)

kgaikwad
kgaikwad suggested changes Sep 30, 2019
View reviewed changes
Copy link
Member

@kgaikwad kgaikwad left a comment

Choose a reason for hiding this comment

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

Few code changes are required to run this check without any failure.

definitions/checks/foreman_facts_names.rb Outdated Show resolved Hide resolved
definitions/checks/foreman_facts_names.rb Outdated
fact_values = 0
end
assert(fact_values < max,
"Host (ID #{host_id}) has #{fact_values} fact values. This can cause slow fact processing.",
Copy link
Member

Choose a reason for hiding this comment

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

How about adding max value in the message so that user will know on what basis it is showing a particular host record?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

definitions/checks/foreman_facts_names.rb Outdated
@@ -0,0 +1,29 @@
class Checks::ForemanFactsNames < ForemanMaintain::Check
Copy link
Member

Choose a reason for hiding this comment

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

We have a foreman directory under checks for foreman specific checks.
To have a consistency, could you move this check under same directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

kgaikwad
kgaikwad reviewed Sep 30, 2019
View reviewed changes
definitions/checks/foreman_facts_names.rb Outdated
host_id = result['host_id']
fact_values = result['fact_values']
else
fact_values = 0
Copy link
Member

Choose a reason for hiding this comment

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

You can remove else part here & run assert inside if.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • aee9dcc must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • aee9dcc must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

@lzap
Copy link
Member Author

lzap commented Oct 3, 2019

Rebased.

@kgaikwad
Copy link
Member

kgaikwad commented Oct 3, 2019

Thank you @lzap.
Rubocop is not happy :-) othewise looks good to me.

@mbacovsky, @upadhyeammit, do you have any comments?

@lzap
Copy link
Member Author

lzap commented Oct 3, 2019

Made her happy.

upadhyeammit
upadhyeammit approved these changes Oct 23, 2019
View reviewed changes
Copy link
Contributor

@upadhyeammit upadhyeammit left a comment

Choose a reason for hiding this comment

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

LGTM !

@lzap
Copy link
Member Author

lzap commented Oct 30, 2019

Any chance to get this in for 1.24?

@mbacovsky
Copy link
Member

@lzap the PR looks good to merge. As it is just raising warning we can probably merge it without testing on user data which is out of our capacity right now. I'm currently investigating what is causing the test failures.
We may consider building new version of foreman-maintain with this patch and getting it to 1.24.

@kgaikwad
Copy link
Member

@lzap,
Yes, almost there. Test jobs are failing with error - uninitialized constant Checks::Foreman (NameError)

Once resolved, it's ready to merge.

@lzap
Copy link
Member Author

lzap commented Nov 21, 2019

Done, cheers.

kgaikwad
kgaikwad approved these changes Nov 21, 2019
View reviewed changes
Copy link
Member

@kgaikwad kgaikwad left a comment

Choose a reason for hiding this comment

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

Tests are green 🎉
Thank you @lzap for quick update 👍 👌

Tested this check with dummy values not with real user data.
Thank you @mbacovsky, @upadhyeammit for review.
Merging this pull-request.

@kgaikwad kgaikwad merged commit 8a0526d into theforeman:master Nov 21, 2019
@lzap lzap deleted the fact-names-27075 branch March 19, 2020 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@upadhyeammit upadhyeammit upadhyeammit approved these changes

@kgaikwad kgaikwad kgaikwad approved these changes

Assignees
No one assigned
Labels
Needs testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lzap @theforeman-bot @kgaikwad @mbacovsky @upadhyeammit