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

additional error checking in ping script #553

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

clutch2sft
Copy link

Made command build "more correct"

Argument Splitting: Usually, each argument and the command itself are separate elements in the list. If any of the flags or options require values (like -c 5), those should usually also be separate elements in the list.

Implement try catch block in _command to catch any errors from the shell.

Returning the commands exit code.

Check exit code and if it is NOT zero(0)=all the pinged hosts are reachable -or- one(1)=Some hosts were unreachable. Then raise an error to improve (speed up) troubleshooting.

Checks:

  • I have manually tested the proposed changes
  • I have written new test cases to avoid regressions (if necessary)
  • I have updated the documentation (e.g. README.rst)

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

the file is getting entirely rewritten, I am not sure why, can you please fix that? We cannot perform a proper review if we do not see what exactly is getting changed.

@clutch2sft
Copy link
Author

the file is getting entirely rewritten, I am not sure why, can you please fix that? We cannot perform a proper review if we do not see what exactly is getting changed.

I can see the individual changes in two commits in the commits tab. If you can't see those there I can copy into this conversation. Thanks! Ggg

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

the file is getting entirely rewritten, I am not sure why, can you please fix that? We cannot perform a proper review if we do not see what exactly is getting changed.

I can see the individual changes in two commits in the commits tab. If you can't see those there I can copy into this conversation. Thanks! Ggg
Thanks for explaining.

The second commit seems to be causing issues, could you please remove that commit and add the change that was meant to be in that commit to the previous one using git commig --amend? That will update the first commit.

@clutch2sft clutch2sft force-pushed the master branch 3 times, most recently from e4625e2 to 2ff0723 Compare October 27, 2023 17:48
@clutch2sft
Copy link
Author

I think the commit issue is resolved. Thanks.

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