-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add gather facts function #383
Conversation
collect_python_location(extended_node) | ||
|
||
|
||
def collect_sysconfig(extended_node, client): |
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 is better not to mix up the "sonar" logic with the general upgrade flow logic (its steps).
Please move this logic to some sonar_utils.py file that will contain the "sonar" logic, that will only return a map with the sysconfig settings (i.e. without changing the extended_node structure). In additional, I think it is better approach to not save "sysconfig" on the extended node, but to save only the info we need from it, as you did with extended_node['python_location'] and the rest of the code use extended_node["python_location"] and not familiar with its key in the sysconfig file. Just to mention that in case of adding a field to the extended_node, then it suppose to be initialized as undefined in the crate_extended_node func so the extended_node structure will be well defined (it is irrelevant now when not saving sysconfig on the node).
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.
If collecting this config is no longer to be a "step", then why was getting the python location a step previously? This does more, and especially now the python location just uses this data to get the path.
I'm not sure this PR is the place to refactor how you set up steps and where functions are located, I'm just trying to get thing set up currently so that I can actually run the preflight/postflight that is bundled with sonar.
Hi Chris, I think it is a good idea. I had few comments on the code and the test, please take a look on the main code comments before the tests comments. |
Hi Everyone, @jsonar-cpapke @sivan-hajbi-imperva - what's left to be done for this PR before it gets approved/merged? |
I approved it.
בתאריך 22 בפבר׳ 2024 15:57, Darya Abdollahzadeh ***@***.***> כתב:
CAUTION: This message was sent from outside the company. Do not click links or open attachments unless you recognize the sender and know the content is safe.
Hi Everyone, @jsonar-cpapke<https://github.com/jsonar-cpapke> @sivan-hajbi-imperva<https://github.com/sivan-hajbi-imperva> - what's left to be done for this PR before it gets approved/merged?
—
Reply to this email directly, view it on GitHub<#383 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A4C5U2275U3FTEF44M4ZFH3YU5FFBAVCNFSM6AAAAABDFP43PWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJZGUYDGNZYGY>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
…-------------------------------------------
This message is confidential. If you believe you received this message in error, please inform the sender and delete this message and all attachments.
|
Gather sysconfig, and use that for the python location.
This allows for more future checks based on the jsonar sysconfig file (like based on current version, or current data dir)