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 #20468: adapt rudder-compliance script for python 3 #687
Fixes #20468: adapt rudder-compliance script for python 3 #687
Conversation
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 have added 2/3 things that can be improved.
for j in directives_compliance_output[i]: | ||
directive_name = id_to_name(directives, j) | ||
directive_compliance = directives_compliance_output[i][j] | ||
|
||
print '** ' + directive_name + ': ' + str(directive_compliance) + '%' | ||
print('** ' + directive_name + ': ' + str(directive_compliance) + '%') |
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.
To make it more readable, you should use the f-strings
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.
f-string was introduced in Python 3.6, yet some distro are not using it (sles12 is using python 3.4)
i'm looking for an alternative
|
||
csv_content.append([ i, j, directive_compliance ]) | ||
|
||
print '' | ||
print('') |
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.
To display an empty line, a "print()" is enough
@@ -20,7 +20,7 @@ from requests.packages.urllib3.exceptions import InsecureRequestWarning, Insecur | |||
# Configuration - BEGIN | |||
api_version = "latest" | |||
api_url = "https://rudder.example.com/rudder/api" | |||
api_token = "dTxvl4eL8p3YqvwefVbaJLdy8DyEt7Vw" | |||
api_token = "z7v9j0hsNnrjRvBnLvL01ioUwVJpTgHs" |
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.
Why specify in hard the API key and the API url ? Why not do it via options to the script and give it default values (like: "https://localhost/rudder" for the endpoint)?
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.
because it was done like that :)
I don't have a much better reason than that
rules_compliance_output.append([id, compliance]) | ||
|
||
# Human-readable output | ||
for i in rules_compliance_output: | ||
print "* Rule " + id_to_name(rules, i[0]) + ": " + str(i[1]) + '%' | ||
print("* Rule " + id_to_name(rules, i[0]) + ": " + str(i[1]) + '%') |
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.
To make it more readable, you should use the f-strings
PR updated with a new commit |
PR updated with a new commit |
Everything is ok, @amousset you can merge this PR. |
@@ -4,40 +4,45 @@ | |||
# Name: rudder-compliance | |||
# Synopsis: Get compliance output Rules and Directives | |||
# Requirements: requests Python module | |||
# Author: Matthieu Cerda <matthieu.cerda@normation.com> |
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.
😢
api_url = "https://rudder.example.com/rudder/api" | ||
api_token = "dTxvl4eL8p3YqvwefVbaJLdy8DyEt7Vw" | ||
default_api_url = "https://rudder.example.com/rudder/api" | ||
default_api_token = "z7v9j0hsNnrjRvBnLvL01ioUwVJpTgHs" |
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 think it does not make sense to have default values 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.
as I think of it why not it clearly shows where to replace the values
OK, squash merging this PR |
479175c
to
0f79308
Compare
https://issues.rudder.io/issues/20468