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

Add gather facts function #383

Merged
merged 6 commits into from
Feb 22, 2024
Merged

Add gather facts function #383

merged 6 commits into from
Feb 22, 2024

Conversation

jsonar-cpapke
Copy link
Collaborator

@jsonar-cpapke jsonar-cpapke commented Feb 12, 2024

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)

collect_python_location(extended_node)


def collect_sysconfig(extended_node, client):
Copy link
Collaborator

@sivan-hajbi-imperva sivan-hajbi-imperva Feb 14, 2024

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).

Copy link
Collaborator Author

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.

modules/aws/sonar-upgrader/python_upgrader/upgrade/main.py Outdated Show resolved Hide resolved
modules/aws/sonar-upgrader/python_upgrader/upgrade/main.py Outdated Show resolved Hide resolved
@sivan-hajbi-imperva
Copy link
Collaborator

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.

@darya-abdollahzadeh
Copy link
Collaborator

Hi Everyone, @jsonar-cpapke @sivan-hajbi-imperva - what's left to be done for this PR before it gets approved/merged?

@sivan-hajbi-imperva
Copy link
Collaborator

sivan-hajbi-imperva commented Feb 22, 2024 via email

@jsonar-cpapke jsonar-cpapke merged commit aef62cb into dev Feb 22, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants