{"action":"created","issue":{"url":"https://api.github.com/repos/google/capirca/issues/67","repository_url":"https://api.github.com/repos/google/capirca","labels_url":"https://api.github.com/repos/google/capirca/issues/67/labels{/name}","comments_url":"https://api.github.com/repos/google/capirca/issues/67/comments","events_url":"https://api.github.com/repos/google/capirca/issues/67/events","html_url":"https://github.com/google/capirca/pull/67","id":158483276,"number":67,"title":"LARGE PR: all of my development branch changes","user":{"login":"jzohrab","id":1637133,"avatar_url":"https://avatars.githubusercontent.com/u/1637133?v=3","gravatar_id":"","url":"https://api.github.com/users/jzohrab","html_url":"https://github.com/jzohrab","followers_url":"https://api.github.com/users/jzohrab/followers","following_url":"https://api.github.com/users/jzohrab/following{/other_user}","gists_url":"https://api.github.com/users/jzohrab/gists{/gist_id}","starred_url":"https://api.github.com/users/jzohrab/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jzohrab/subscriptions","organizations_url":"https://api.github.com/users/jzohrab/orgs","repos_url":"https://api.github.com/users/jzohrab/repos","events_url":"https://api.github.com/users/jzohrab/events{/privacy}","received_events_url":"https://api.github.com/users/jzohrab/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2016-06-04T02:39:12Z","updated_at":"2016-06-04T11:55:37Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/google/capirca/pulls/67","html_url":"https://github.com/google/capirca/pull/67","diff_url":"https://github.com/google/capirca/pull/67.diff","patch_url":"https://github.com/google/capirca/pull/67.patch"},"body":"This is a very large pull request, it will likely be easiest to discuss this in a conference call.\r\n\r\nI created this branch (merge_master) from my development branch, https://github.com/jzohrab/capirca/commits/development, and then merged google Capirca master into it.  I first fixed the merge conflicts, and then did individual commits to get the tests running and passing.  List of these commits: https://github.com/jzohrab/capirca/commits/merge_master.  If needed, I could squash all of these fixes down into one _giant_ commit, but felt it would be better to have them separate at first.\r\n\r\nAll the unit and integration tests in this branch currently run and pass with \"python -m unittest discover\".  This includes the tests I added, as well as the old tests and the new ones added to master.  \"Ran 497 tests in 52.177s   ...  OK\", which is pretty nice.  Thanks for adding all the tests.\r\n\r\nCapirca/master was updated as I did this work, and so I merged that work, as at commit 840616e, in here as well.\r\n\r\n# Changes\r\n\r\nThis PR merges almost all of the changes I've made in my development branch (see an overview of these changes at the list of closed pull requests into my dev branch: https://github.com/jzohrab/capirca/pulls?q=is%3Apr+is%3Aclosed).\r\n\r\n## Biggest changes\r\n\r\n* parsing of the Capirca data is separated from the model\r\n* added a yaml policy parser, and a \"policy_format\" flag so users can choose which format to parse.\r\n* added a YamlNaming object, similar to the existing Naming object for consistency\r\n* added characterization tests for the existing code, and test coverage for the above\r\n\r\nNote that the yaml format does _not_ have complete feature parity with the capirca format, this is documented in doc/policy_format_yml.md, and perhaps should be iterated on.  We're using this format at the moment for an internal POC project.\r\n\r\nI backed out some of my changes (eg the introduction of the AclGen object in the series of commits in my PR 2, commit d120259) since I chose to use the new aclgen.py code in Capirca master, following discussion with Rob (ankenyr), so even though those commits are in the rev history the content is overwritten with the merge at commit b48fba9.\r\n\r\n## Smaller changes\r\n\r\n* added cisco nexus support\r\n* cleaned up some logging, for people wishing to use the library modules as a third-party library (which we're doing in a project at the moment)\r\n* pip requirements\r\n* fixed the new tests from Google to use the policyparser object\r\n* added symlinks for test discovery (see \"dummy symlinks\" note below)\r\n* some documentation clean-up\r\n\r\n\r\n# Notes\r\n\r\n## Choosing and documenting one test method\r\n\r\nI added tests when the public release of Capirca had no automated tests.  These are in \"test\" and modules are named \"test_...\" to allow for test discovery.\r\n\r\nThe newly released master code has some tests under \"tests\".  We should combine these, but I've left them separate for now as I didn't want to introduce yet more churn, and I didn't know what was a good choice.\r\n\r\nI also recommend that we change the code so that all of them can be run from a single command-line call - perhaps we could rename the modules in the \"tests\" directory from \"xxx_test.py\" to \"test_xxx.py\" to facilitate test discovery.\r\n\r\n## Dummy symlinks\r\n\r\nSymlinks added in commit 03f1e1712.\r\n\r\n'python -m unittest discover' requires that all tests be named 'test_xxx.py'. Renaming all of the 'tests' files to this naming convention would be extra thrash,\r\nand perhaps would not be the right decision for the team/project.  This hack lets us run all of the tests in one shot using test discovery, at least until the CI direction is set.\r\n\r\n## Potential issues/problems with the code\r\n\r\n### Error not raised on `naming.NoDefinitionsError` in aclgen:\r\n\r\nNote that files are still generated even if there is a bad def’s directory:\r\n\r\n```\r\n  except naming.NoDefinitionsError:\r\n    logging.fatal('bad definitions directory: %s', FLAGS.definitions_directory)\r\n```\r\n\r\nand the error is not rethrown.\r\n\r\n\r\n### Potential implementation issue with aclgen.DescendRecursively\r\n\r\nI'm not sure what the current DescendRecursively function is aiming to do.  It appears to ignore nested directories, eg the below:\r\n\r\n```\r\npol\r\n  - some_pol.pol\r\n  nested\r\n    - some_nested_dir.pol  << IGNORED\r\n```\r\n\r\nI removed that from the characterization data test case.  We should add some tests around that to ensure it's doing exactly what you expect.\r\n\r\n\r\n---\r\n\r\nThanks, and sorry for the giant wall of text.  Let me know when you're free to discuss, it would be great to get these into master.\r\n"},"comment":{"url":"https://api.github.com/repos/google/capirca/issues/comments/223751690","html_url":"https://github.com/google/capirca/pull/67#issuecomment-223751690","issue_url":"https://api.github.com/repos/google/capirca/issues/67","id":223751690,"user":{"login":"jzohrab","id":1637133,"avatar_url":"https://avatars.githubusercontent.com/u/1637133?v=3","gravatar_id":"","url":"https://api.github.com/users/jzohrab","html_url":"https://github.com/jzohrab","followers_url":"https://api.github.com/users/jzohrab/followers","following_url":"https://api.github.com/users/jzohrab/following{/other_user}","gists_url":"https://api.github.com/users/jzohrab/gists{/gist_id}","starred_url":"https://api.github.com/users/jzohrab/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jzohrab/subscriptions","organizations_url":"https://api.github.com/users/jzohrab/orgs","repos_url":"https://api.github.com/users/jzohrab/repos","events_url":"https://api.github.com/users/jzohrab/events{/privacy}","received_events_url":"https://api.github.com/users/jzohrab/received_events","type":"User","site_admin":false},"created_at":"2016-06-04T11:55:37Z","updated_at":"2016-06-04T11:55:37Z","body":"Thanks @supertylerc - I made some changes to the class to reduce the impact on the cisco tests that google added, and must have broken the Nexus support.  It was working in the dev branch, so I'll disable Nexus support in this branch, work to restore the output, and will likely open a new PR."}}
{"actor":{"display_login":"jzohrab"}}