cnaas-nms icon indicating copy to clipboard operation
cnaas-nms copied to clipboard

Add support for branch refspecs in Git repo urls

Open lunkwill42 opened this issue 3 years ago • 4 comments

This will parse any fragment present in a Git repo url as a branch refspec, and make sure to clone that branch when the repo is first cloned.

I would have liked to add some unit tests for the new function, but I am unable to run said unit tests in isolation, because the existing code base carries the sad code smell of simple module imports causing side-effects: Just attempting to run the tests present in cnaas_nms.db.tests.test_git ultimately causes the cnaas_nms.db.session module to be imported, and this module tries to connect to a running database at import time. This problem should be fixed separately.

Fixes #222

lunkwill42 avatar Nov 16 '21 14:11 lunkwill42

Codecov Report

Merging #223 (8fe1db1) into develop (52840ef) will decrease coverage by 4.36%. The diff coverage is 100.00%.

:exclamation: Current head 8fe1db1 differs from pull request most recent head b5d5b0f. Consider uploading reports for the commit b5d5b0f to get more accurate results

@@             Coverage Diff             @@
##           develop     #223      +/-   ##
===========================================
- Coverage    63.78%   59.42%   -4.37%     
===========================================
  Files           63       61       -2     
  Lines         6627     6186     -441     
===========================================
- Hits          4227     3676     -551     
- Misses        2400     2510     +110     
Impacted Files Coverage Δ
src/cnaas_nms/db/git.py 55.04% <100.00%> (-8.24%) :arrow_down:
src/cnaas_nms/api/linknet.py 19.56% <0.00%> (-46.93%) :arrow_down:
src/cnaas_nms/confpush/underlay.py 19.60% <0.00%> (-37.26%) :arrow_down:
src/cnaas_nms/tools/jinja_filters.py 14.70% <0.00%> (-22.03%) :arrow_down:
src/cnaas_nms/db/session.py 75.00% <0.00%> (-9.85%) :arrow_down:
src/cnaas_nms/db/linknet.py 79.71% <0.00%> (-9.32%) :arrow_down:
src/cnaas_nms/api/app.py 68.42% <0.00%> (-8.51%) :arrow_down:
src/cnaas_nms/confpush/init_device.py 50.11% <0.00%> (-7.88%) :arrow_down:
src/cnaas_nms/api/repository.py 54.76% <0.00%> (-7.15%) :arrow_down:
... and 33 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Nov 16 '21 15:11 codecov[bot]

Needs some documentation to describe the URL format, but looks simple and clean!

indy-independence avatar Nov 25 '21 12:11 indy-independence

Needs some documentation to describe the URL format, but looks simple and clean!

The first step to documenting this simple function is adding tests for it. Things have happened since I last visited this PR. However, on the current develop branch, unit testing without weird dependencies still proves to be problematic:

To run just a simple test method on the introduced parse_repo_url function, seems to require that I place a JWT certificate in /opt/cnaas/jwtcert/public.pem, which is just ridiculous:

[2022-09-14 15:57:51,457] DEBUG in settings: Loaded settings_fields module from bundled cnaas-nms
[2022-09-14 15:57:51,514] ERROR in pki: Specified cafile is not a file: /opt/cnaas/cacert/rootCA.crt
[2022-09-14 15:57:51,515] WARNING in pki: Accepting unverified TLS certificates
[2022-09-14 15:57:52,555] DEBUG in settings: Loaded settings_fields module from bundled cnaas-nms
E
======================================================================
ERROR: test suite for <module 'cnaas_nms.db.tests.test_git' from '/Users/mvold/d/cnaas-nms/src/cnaas_nms/db/tests/test_git.py'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mvold/d/cnaas-nms/.ve/lib/python3.7/site-packages/nose/suite.py", line 210, in run
    self.setUp()
  File "/Users/mvold/d/cnaas-nms/.ve/lib/python3.7/site-packages/nose/suite.py", line 293, in setUp
    self.setupContext(ancestor)
  File "/Users/mvold/d/cnaas-nms/.ve/lib/python3.7/site-packages/nose/suite.py", line 316, in setupContext
    try_run(context, names)
  File "/Users/mvold/d/cnaas-nms/.ve/lib/python3.7/site-packages/nose/util.py", line 471, in try_run
    return func()
  File "/Users/mvold/d/cnaas-nms/src/cnaas_nms/__init__.py", line 5, in setup_package
    import cnaas_nms.api.app
  File "/Users/mvold/d/cnaas-nms/src/cnaas_nms/api/app.py", line 90, in <module>
    sys.exit(1)
SystemExit: 1
-------------------- >> begin captured stdout << ---------------------
Error in add_event: Error 61 connecting to 127.0.0.1:6379. Connection refused.
Error in add_event: Error 61 connecting to 127.0.0.1:6379. Connection refused.
Error in add_event: Error 61 connecting to 127.0.0.1:6379. Connection refused.
Error in add_event: Error 61 connecting to 127.0.0.1:6379. Connection refused.
Could not load public JWT cert from api.yml config: [Errno 2] No such file or directory: '/opt/cnaas/jwtcert/public.pem'

--------------------- >> end captured stdout << ----------------------
-------------------- >> begin captured logging << --------------------
git.cmd: DEBUG: Popen(['git', 'version'], cwd=/Users/mvold/d/cnaas-nms/src/cnaas_nms/db/tests, universal_newlines=False, shell=None, istream=None)
git.cmd: DEBUG: Popen(['git', 'version'], cwd=/Users/mvold/d/cnaas-nms/src/cnaas_nms/db/tests, universal_newlines=False, shell=None, istream=None)
passlib.utils.compat: DEBUG: loaded lazy attr 'SafeConfigParser': <class 'configparser.ConfigParser'>
passlib.utils.compat: DEBUG: loaded lazy attr 'NativeStringIO': <class '_io.StringIO'>
passlib.utils.compat: DEBUG: loaded lazy attr 'BytesIO': <class '_io.BytesIO'>
cnaas-nms: DEBUG: Loaded settings_fields module from bundled cnaas-nms
cnaas-nms: ERROR: Specified cafile is not a file: /opt/cnaas/cacert/rootCA.crt
cnaas-nms: WARNING: Accepting unverified TLS certificates
cnaas-nms: DEBUG: Loaded settings_fields module from bundled cnaas-nms
--------------------- >> end captured logging << ---------------------

----------------------------------------------------------------------
Ran 0 tests in 1.069s

FAILED (errors=1)

error in setup context
Traceback (most recent call last):
  File "/Users/mvold/d/cnaas-nms/.ve/lib/python3.7/site-packages/nose/suite.py", line 210, in run
    self.setUp()
  File "/Users/mvold/d/cnaas-nms/.ve/lib/python3.7/site-packages/nose/suite.py", line 293, in setUp
    self.setupContext(ancestor)
  File "/Users/mvold/d/cnaas-nms/.ve/lib/python3.7/site-packages/nose/suite.py", line 316, in setupContext
    try_run(context, names)
  File "/Users/mvold/d/cnaas-nms/.ve/lib/python3.7/site-packages/nose/util.py", line 471, in try_run
    return func()
  File "/Users/mvold/d/cnaas-nms/src/cnaas_nms/__init__.py", line 5, in setup_package
    import cnaas_nms.api.app
  File "/Users/mvold/d/cnaas-nms/src/cnaas_nms/api/app.py", line 90, in <module>
    sys.exit(1)
Exception: 1

-------------------- >> begin captured logging << --------------------
git.cmd: DEBUG: Popen(['git', 'version'], cwd=/Users/mvold/d/cnaas-nms/src/cnaas_nms/db/tests, universal_newlines=False, shell=None, istream=None)
git.cmd: DEBUG: Popen(['git', 'version'], cwd=/Users/mvold/d/cnaas-nms/src/cnaas_nms/db/tests, universal_newlines=False, shell=None, istream=None)
passlib.utils.compat: DEBUG: loaded lazy attr 'SafeConfigParser': <class 'configparser.ConfigParser'>
passlib.utils.compat: DEBUG: loaded lazy attr 'NativeStringIO': <class '_io.StringIO'>
passlib.utils.compat: DEBUG: loaded lazy attr 'BytesIO': <class '_io.BytesIO'>
cnaas-nms: DEBUG: Loaded settings_fields module from bundled cnaas-nms
cnaas-nms: ERROR: Specified cafile is not a file: /opt/cnaas/cacert/rootCA.crt
cnaas-nms: WARNING: Accepting unverified TLS certificates
cnaas-nms: DEBUG: Loaded settings_fields module from bundled cnaas-nms
--------------------- >> end captured logging << ---------------------

Incidentally, I also find this to be kind of silly. What is the point of the sys.exit(1) in this case? Also produces a pretty bad side-effect during testing...: https://github.com/SUNET/cnaas-nms/blob/9e08e7fa5e02deeb7c77b443b9f27976a5eb2f46/src/cnaas_nms/api/app.py#L86-L90

Ideally, the test suite should be able to set up a test environment where any necessary configuration is present, and not be dependent on files being present in specific absolute paths. Also, ideally, no such config should be necessary just to test the parse_repo_url() function. It mostly boils down (again) to code that is setting up application state as a side-effect of simply importing a Python module.

You know the codebase better than me, @indy-independence; what do you think would be the best strategy to mitigate this? I really want to get on with adding value to CNaaS-NMS, but the simple things are made so hard...

lunkwill42 avatar Sep 14 '22 14:09 lunkwill42

Needs some documentation to describe the URL format, but looks simple and clean!

Anyhoo, I added some suggested docs. Since there are multiple GITREPO settings where this can be used, I suggested adding a new section to the config docs.

lunkwill42 avatar Sep 14 '22 14:09 lunkwill42