-
Notifications
You must be signed in to change notification settings - Fork 50
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
SSM Automation document graph visualization to help with branching in complex workflows #24
base: master
Are you sure you want to change the base?
Conversation
…t to DOT language
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 added a number of comments in-line. Also as a general thought: the placement of convert_document_to_dot_graph() in Documents/Automation/Testing/ssm_testing.py does not seem appropriate. The method isn't related to testing nor does it need to operate on an class instance (hence static). It may be worthwhile to make a separate location for tools.
json_doc = json.load(jsonfile, object_pairs_hook=OrderedDict) | ||
|
||
# Initializating the graph variable with the document description and the default Start and End nodes | ||
graph = [] |
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.
Lines 160-164 could be replaced with a single list literal.
# or save the current step information to be able to create the edge when inspecting the next available step | ||
elif step["onFailure"] == "Continue": | ||
if "nextStep" in step: | ||
label="onFailure color=\"red\"" |
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.
PEP8: missing spaces around the = operator.
label="onFailure color=\"red\"" | ||
if "isCritical" in step: | ||
if step["isCritical"] == "false": | ||
label="onFailure" |
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.
PEP8: missing spaces around the = operator.
step["name"], step["nextStep"], label)) | ||
else: | ||
add_edge_from_previous_step = True | ||
label="onFailure color=\"red\"" |
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.
PEP8: missing spaces around the = operator.
previous_step_name = step["name"] | ||
# Lastly, retrieve the next step from onFailure directly | ||
else: | ||
label="onFailure color=\"red\"" |
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.
PEP8: missing spaces around the = operator.
label="onFailure color=\"red\"" | ||
if "isCritical" in step: | ||
if step["isCritical"] == "false": | ||
label="onFailure" |
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.
PEP8: missing spaces around the = operator.
del choice["NextStep"] | ||
# Removing first and last character from the choice (that removes the curly brackets), | ||
# escaping and adding a new line for each comma) | ||
label = "\"{}\"".format(json.dumps(choice)[1:-1].replace('", "','"\\l"').replace('"','\\"')) |
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.
PEP8: missing spaces after the commas.
json_doc = json.load(jsonfile, object_pairs_hook=OrderedDict) | ||
|
||
# Initializating the graph variable with the document description and the default Start and End nodes | ||
graph = [] |
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 recommend choosing a different variable name. "graph" has its own meaning in the context of programming/computer science and the type of this variable is actually a list. Consider "dot_lines", "document_lines", or something else.
elif step["onFailure"] == "Continue": | ||
if "nextStep" in step: | ||
label="onFailure color=\"red\"" | ||
if "isCritical" in step: |
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.
The two if lines could be condensed into one:
if "isCritical" in step and step["isCritical"] == "false":
This also applies to lines 238-239.
break | ||
|
||
# If action is aws:branch, checking all choices to visualize each branch | ||
if step["action"] == "aws:branch": |
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.
SSM's branching feature supports the and, or, and not logical operators. I think it would be valuable to support them here as well.
Hi @alemartini any plan on tackling the requested changes any time soon? :) |
Issue #, if available:
Description of changes: SSMTester static method convert_document_to_dot_graph converts an SSM Automation document from a JSON format to a directional graph in DOT language [1].
For each document, I can now have an additional script under Setup, and update the Makefile to generate a visual representation of the workflow to analyze branching on onFailure and onSuccess.
As an example, I added the script to AWS-DetachEBSVolume. To test:
[1] https://www.graphviz.org/doc/info/lang.html
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.