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

Jobs list error ; Closes #262 #263

Merged
merged 4 commits into from
Jan 7, 2025

Conversation

pranjalg1331
Copy link

Description

I have corrected the key value error and added a condition to check it the playbook_to_execute field is iterable.
Please include a summary of the change.

Related issues

Please add related issues.

Type of change

Please delete options that are not relevant.

  • [x ] Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist

  • [ x] The pull request is for the branch develop
  • I have added tests in the Tests folder.
  • The tests gave 0 errors.
  • [ x] Black gave 0 errors.
  • [ x] Flake gave 0 errors.
  • I squashed the commits into a single one. (optional, they will be squashed anyway by the maintainer)

please follow these rules

  • If your changes decrease the overall tests coverage (you will know after the Codecov CI job is done), you should add the required tests to fix the problem
  • Everytime you make changes to the PR and you think the work is done, you should explicitly ask for a review

Real World Example

Please delete if the PR is for bug fixing.
Otherwise, please provide the resulting raw JSON of a finished analysis (and, if you like, a screenshot of the results). This is to allow the maintainers to understand how the analyzer works.

@mlodic
Copy link
Member

mlodic commented Jan 2, 2025

thank for finding the issue but actually this does not fix the problem cause the key is being accessed anyway. You should write in to safe check it

@pranjalg1331
Copy link
Author

I have added a further check in case the key is not present. Here is the resultant output.
image

Signed-off-by: pranjalg1331 <[email protected]>
Comment on lines +127 to +129
el.get("playbook_to_execute", [])
if el.get("playbook_to_execute")
else []
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
el.get("playbook_to_execute", [])
if el.get("playbook_to_execute")
else []
el["playbook_to_execute"]
if "playbook_to_execute" in el
else []

Copy link
Member

Choose a reason for hiding this comment

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

this is the best way to do that

Copy link
Author

Choose a reason for hiding this comment

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

@mlodic, sir this would not check if el["playbook_to_execute"] is iterable or not.

Copy link
Author

@pranjalg1331 pranjalg1331 Jan 4, 2025

Choose a reason for hiding this comment

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

image

So when it is null, we will get an error.

Copy link
Member

@mlodic mlodic Jan 7, 2025

Choose a reason for hiding this comment

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

oh good catch. I think that this is an imperfection of the APIs because either you have the key populated or you don't have that key at all. Having the key available and set as null does not make any sense, it is wasted data and causes problems in handling the data, like the one you found. So well, we have to stick with your solution. Thank you

@mlodic mlodic merged commit f33e956 into intelowlproject:develop Jan 7, 2025
4 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.

2 participants