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

Docs were removed? #58

Closed
jzohrab opened this issue May 7, 2016 · 5 comments
Closed

Docs were removed? #58

jzohrab opened this issue May 7, 2016 · 5 comments

Comments

@jzohrab
Copy link
Contributor

jzohrab commented May 7, 2016

In commit ad15397, the doc folder was deleted. Was this intentional?

@ankenyr
Copy link
Contributor

ankenyr commented May 7, 2016

That was not intended. I apologize. I will get those pushed back out shortly. We did remove the third party folder and some of the tools. I hope that did not break anything for you or anyone else. If so let me know and I can try to fix it. Setup.py should have all the dependencies needed.

@jzohrab
Copy link
Contributor Author

jzohrab commented May 7, 2016

Hey @ankenyr, thanks.

That commit is big. Actually, I'm not sure how the code would work ... now that the ply third-party code has been deleted I don't see how the policy parser could work.

I'm still working on https://github.com/jzohrab/capirca/tree/development and doing my best to keep my development branch clean and all commits very strict, as I think that all my changes are something that could benefit everyone. I'd very much like to share if possible. I'm making many small but well-documented and tested changes. It would be great to contribute back, but I am afraid that I'm going to start to diverge. I'd be very happy to have a conference call with the team to try to align my efforts to yours, as I feel that even though it may slow us both down in the short term, it would benefit us and others in the long term.

Thanks and regards!

@ankenyr
Copy link
Contributor

ankenyr commented May 7, 2016

Yes the large commit and removal of ply and the other tools from third party is an artifact of some internal requirements. Basically including that without the license was wrong and since it is supported elsewhere we shouldn't be including the files. Updating those lines to from ply import lex/yacc should work. I can make those changes shortly also.

I am happy to have a conference call to talk about this. The differences between the internal version we have at Google and this version on Github is pretty large. We have tests for everything and some python files are almost 100% different.
Here are some diffstats for some of the more critical files from my point of view.

$ diff aclgen.py <internal_path>tools/aclgen.py | diffstat
unknown | 725 +++++++++++++++++++++++++++++++++++++++++++++-------------------
1 file changed, 521 insertions(+), 204 deletions(-)

$ diff lib/cisco.py <internal_path>lib/cisco.py | diffstat
unknown | 242 ++++++++++++++++++++++++++++++++++++----------------------------
1 file changed, 139 insertions(+), 103 deletions(-)

$ diff lib/aclgenerator.py <internal_path>lib/aclgenerator.py | diffstat
unknown | 85 ++++++++++++++++++++++++++++++----------------------------------
1 file changed, 41 insertions(+), 44 deletions(-)

$ diff lib/juniper.py <internal_path>lib/juniper.py | diffstat
unknown | 363 +++++++++++++++++++++++++++++++++++++++++++++-------------------
1 file changed, 259 insertions(+), 104 deletions(-)

$ diff lib/iptables.py <internal_path>lib/iptables.py | diffstat
unknown | 25 ++++++-------------------
1 file changed, 6 insertions(+), 19 deletions(-)

$ diff lib/nacaddr.py <internal_path>lib/nacaddr.py | diffstat
unknown | 53 ++++++++++++++++++++++++-----------------------------
1 file changed, 24 insertions(+), 29 deletions(-)

$ diff lib/junipersrx.py <internal_path>lib/junipersrx.py | diffstat
unknown | 557 +++++++++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 452 insertions(+), 105 deletions(-)

$ diff lib/naming.py <internal_path>lib/naming.py | diffstat
unknown | 62 +++++++++++++++++++++++++++++++++-----------------------------
1 file changed, 33 insertions(+), 29 deletions(-)

$ diff lib/policy.py <internal_path>lib/policy.py | diffstat
unknown | 444 +++++++++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 355 insertions(+), 89 deletions(-)

It is unfortunate that the code base drifted as much as it did and I am only trying to do the right thing. I want to get our internal code out as quickly as possible. As you see above there are a lot of changes and great improvements. I am happy to take suggestions, this is my first time doing a lot with Github and would like to keep our little community happy. I need to however balance that with keeping our internal stuff working fine and my other responsibilities internal to Google.

@ankenyr
Copy link
Contributor

ankenyr commented May 7, 2016

The doc folder has been replaced and policy.py now has the correct import statements for lex/yacc. Ran it locally as a test. I can either mark this topic as closed and we can continue the discussion on our internal migration on issue #57 or we can continue it here. Like I said before, we have a lot to do and I want to do the right thing.

@jzohrab
Copy link
Contributor Author

jzohrab commented May 9, 2016

Good morning @ankenyr, thanks for the notes.

Understood about the need for balance, we both have our own priorities. Let's try to get our codebases together. If that is too difficult, or if our requirements diverge too much, then we can separate, but I'd like to try to get my work in line with yours, and provide some help for the project.

I'll copy some of the above over to issue 57, and will close this one.

@jzohrab jzohrab closed this as completed May 9, 2016
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

No branches or pull requests

2 participants