Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add support to configure parameters form insights-client.conf #177
base: main
Are you sure you want to change the base?
feat: Add support to configure parameters form insights-client.conf #177
Changes from 7 commits
94a83b6
98455e2
b34b8ac
aa4ab6d
7eb37a5
efcc72e
e2395bc
ff0199b
1600b96
808dd18
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@ptoscano Is this value built-in to the insights client? where does it come from?
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 the default value from
/etc/insights-client/insights-client.conf
: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.
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.
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.
Does there need to be a check first for
rhc_insights.file_redaction is defined
?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.
AFAICS
rhc_insights.file_redaction
is defined indefaults/main.yml
. Asdefaults/main.yml
as well astasks/insights-client.yml
are part of one role and shipped together I do not see a scenario whererhc_insights.file_redaction
would not be defined. So with my current knowledge and understanding I would say there is no need to check first forrhc_insights.file_redaction is defined
.I see that most if not all tasks in
tasks/insights-client.yml
check whether variables are defined although all of these variables are defined indefaults/main.yml
. TBH I do not understand why these checks are necessary. Are they in place for "just in case" scenarios or is there some reason I do not see?Following my own argument I would suggest removing all the
var is defined
checks where the var is defined indefaults/main.yml
as the check is not striclty necessary. But I might miss a good reason here.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.
That unfortunately doesn't mean anything.
What if I run the role like this?
Defining a host variable takes precedence over settings in defaults/main.yml. The same applies to setting the variable at the play level or as one of the
vars
in theinclude_role
call. Settingrhc_insights
in this way completely replaces the existing contents defined in defaults/main.yml. https://docs.ansible.com/ansible/latest/reference_appendices/general_precedence.html#variablesSo - while "top level" variables like
rhc_insights
will always be defined, their "sub-parameters" may or may not be defined - that is -rhc_insights is defined
will always returntrue
, butrhc_insights.file_redaction is defined
can returntrue
orfalse
.If you don't believe me, try running the role with different values and see what happens.
Yes, and if some checks are missing, they should be added in another PR.
TBH, I'm really not sure why "sub-parameter" keys and values are defined in
defaults/main.yml
, except possibly as a convenience when the user does not provide a value forrhc_insights
at all. There is no mechanism in Ansible like "merge the default dict valued parameterrhc_insights
defined indefaults/main.yml
with therhc_insights
parameter given by the user". If the user definesrhc_insights
, then the value indefaults/main.yml
is completely replaced.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.
…
Thank you for reminding me about variable precedence and giving me a good example when things will break. I didn't find this as my own tests are still flawed and incomplete.
I cannot thank you enough for bearing with me and share your thoughts and insights on this as it is a great learning opportunity for myself.
Well, putting all variables, parameters and arguments accepted by the role into
defaults/main.yml
gives users a single place to look what input parameters are accepted. That's in alignment with this section of the automation good practices. And I understood that every condition needs to check whether a variable is defined or not as the whole dict defineddefaults/main.yml
is replaced when users specify this elsewhere according to variable precedence.I'm going to check the conditionals in the tasks I've added so far. Please allow me some time to go through them.
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.
Does there need to be a check first for
rhc_insights.file_content_redaction
is defined?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.
Is it possible that this or any of the below values will need to be quoted? will it be possible that the values will contain yaml metacharacters?
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.
For patterns (with regex) that would be possible as the examples in YAML-style denylist configuration for Red Hat Insights Client show characters that need to be escaped.
For keywords, I tend to escape them just to be sure. Do you agree?
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.
This is tricky - afaik there is no "regex quote" or "regex escape" filter in Jinja or Ansible (like there is for shell metacharacters and the
quote
filter). If you use- "{{ pattern }}"
I think this will cause problems ifpattern
contains"
- same with'{{ pattern }}'
and'
- however, this might be safe: