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

Build a graph of resources. Use it to detect circular dependencies #1391

Merged
merged 10 commits into from
Mar 7, 2020

Conversation

miparnisari
Copy link
Contributor

@miparnisari miparnisari commented Feb 29, 2020

Changes:

While debugging I found it very useful to use networkx.drawing.nx_pydot.write_dot(self.graph, path) which creates a visual representation of the graph. I think it would be useful for customers as well. But I wasn't sure whether to include it in this PR because it needs another dependency, pydot and it would need some work to make it a fancy graph.

@PatMyron
Copy link
Contributor

PatMyron commented Feb 29, 2020

While debugging I found it very useful to use networkx.drawing.nx_pydot.write_dot(self.graph, path) which creates a visual representation of the graph. I think it would be useful for customers as well. But I wasn't sure whether to include it in this PR because it needs another dependency, pydot and it would need some work to make it a fancy graph.

Agreed, I think just documenting how to do that here is a nice compromise:

add networkx.drawing.nx_pydot.write_dot(self.graph, 'graph') to the end of __init__(self, cfn) in src/cfnlint/graph.py

cfn-python-lint $ pip3 install pydot       # install pydot
cfn-python-lint $ pip3 install -e . --user # install locally modified version of cfn-lint
cfn-python-lint $ cfn-lint template.yaml   # lint desired template
cfn-python-lint $ pbcopy < graph           # copy graph to paste somewhere like http://www.webgraphviz.com/ or https://sketchviz.com/

Screen Shot 2020-03-01 at 3 54 07 PM

Comment on lines +75 to +76
'networkx~=2.4;python_version>="3.5"',
'networkx~=2.1;python_version<"3.5"'
Copy link
Contributor

Choose a reason for hiding this comment

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

Will need to vet new dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PatMyron PatMyron changed the title Build a graph of resources. Use it to detect circular dependencies. F… Build a graph of resources. Use it to detect circular dependencies Mar 1, 2020
@PatMyron
Copy link
Contributor

PatMyron commented Mar 1, 2020

Nice! From testing, Fn::GetAtt's long form is the last YAML template dependency I could find that's still undetected:

  Resource5:
    Type: AWS::SNS::Topic
    Properties:
      DisplayName:
        Fn::GetAtt: Resource6.TopicName # dependency only caught in !GetAtt form
        # !GetAtt Resource6.TopicName
  Resource6:
    Type: AWS::SNS::Topic
    Properties:
      DisplayName:
        Ref: Resource5

Pushed to your branch if you want to use for testing

src/cfnlint/graph.py Outdated Show resolved Hide resolved
src/cfnlint/graph.py Outdated Show resolved Hide resolved
src/cfnlint/graph.py Outdated Show resolved Hide resolved
@benbridts
Copy link
Contributor

Agreed, I think just documenting how to do that here is a nice compromise:

Another option is to put it behind a command or option and test for the existence of pydot if that's used.

try:
    import pydot
except ImportError:
    # give some feedback here

@kddejong kddejong merged commit 6ef3d95 into aws-cloudformation:master Mar 7, 2020
@miparnisari miparnisari deleted the graph branch March 10, 2020 07:05
@miparnisari miparnisari mentioned this pull request Mar 10, 2020
3 tasks
svenstaro pushed a commit to archlinux/svntogit-community that referenced this pull request Jul 22, 2020
The new python-networkx dependency is from [1]

[1] aws-cloudformation/cfn-lint#1391

git-svn-id: file:///srv/repos/svn-community/svn@602273 9fca08f4-af9d-4005-b8df-a31f2cc04f65
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.

Rule E3004 is not checking for DependsOn declarations
4 participants