Optional API token-based authentication

[rthomson]

This pull request implements optional API token-based authentication when netbox is configured with LOGIN_REQUIRED = True in configuration.py.

netbox.yml example:

netbox:
    main:
        api_url: 'https://localhost/api/dcim/devices/'
        api_token: '1234567890987654321234567890987654321'

where the value of api_token is equivalent to any valid netbox user profile API token.

The api_token key can be omitted from netbox.yml and netbox-as-ansible-inventory behaves as expected (no change to previous behavior).

[rthomson]

Seems there are some tests to update for this PR to pass. Looking into it.

[coveralls]

Coverage decreased (-1.5%) to 97.692% when pulling d72878e756faa9b805b7fb8c74b06fd17fe8c751 on rthomson:master into f88b5f295cf400181133fb99e992a1f98a03adf6 on AAbouZaid:master.

[coveralls]

Coverage decreased (-1.5%) to 97.692% when pulling d72878e756faa9b805b7fb8c74b06fd17fe8c751 on rthomson:master into f88b5f295cf400181133fb99e992a1f98a03adf6 on AAbouZaid:master.

[coveralls]

Coverage decreased (-1.5%) to 97.692% when pulling d72878e756faa9b805b7fb8c74b06fd17fe8c751 on rthomson:master into f88b5f295cf400181133fb99e992a1f98a03adf6 on AAbouZaid:master.

[rthomson]

Got the CI tests to pass by updating tests/test_netbox.py.

I'm not yet clear how on how update the tests so that coveralls passes for key_value = "" and api_url_headers = {}.

I'm new to tests to please bear with me.

[coveralls]

Coverage decreased (-0.8%) to 98.4% when pulling 13302aeeafb511d3cf38c54287ef4755882c3dc9 on rthomson:master into f88b5f295cf400181133fb99e992a1f98a03adf6 on AAbouZaid:master.

[coveralls]

Coverage decreased (-0.8%) to 98.4% when pulling 13302aeeafb511d3cf38c54287ef4755882c3dc9 on rthomson:master into f88b5f295cf400181133fb99e992a1f98a03adf6 on AAbouZaid:master.

[coveralls]

Coverage decreased (-0.8%) to 98.4% when pulling 13302aeeafb511d3cf38c54287ef4755882c3dc9 on rthomson:master into f88b5f295cf400181133fb99e992a1f98a03adf6 on AAbouZaid:master.

[coveralls]

Coverage decreased (-0.8%) to 98.4% when pulling f51e62f38feaed914cb90b87b93898b709616ed8 on rthomson:master into f88b5f295cf400181133fb99e992a1f98a03adf6 on AAbouZaid:master.

[rthomson]

I'm admitting defeat on the final coveralls test. I'm not sure I understand how coveralls works nor how to write tests that will cover the case that coveralls is still complaining about.

[coveralls]

Coverage remained the same at 99.2% when pulling 62bc71e96a4bde16ced1401429b653ab61005b9f on rthomson:master into f88b5f295cf400181133fb99e992a1f98a03adf6 on AAbouZaid:master.

[rthomson]

💯 Finally!

[aabouzaid]

@rthomson Thanks a lot, that looks pretty nice! Could you please switch it develop branch instead master, then I will review and merge.

[rthomson]

@AAbouZaid done!

[aabouzaid]

Cool, I'm doing a quick review now :+1:

[coveralls]

Coverage remained the same at 99.2% when pulling f25cf5fc5f5a2a2d02552c1b49f74f8e6c4a05f8 on rthomson:master into 8607dd5accda2d6611148a410e5db59029e15d13 on AAbouZaid:devel.

[rthomson]

I made some changes to remove the stuff around ignore_key_error that I added. It now relies on the default value without going to sys.exit() if api_token isn't found. I did have to add a conditional to __init__ for NetboxAsInventory to sys.exit() if config was empty to satisfy a test (that I believe was satisfied by the previous logic that was sys.exit()ing when api_token wasn't found).

Not sure if this is any better or not. Also, I'm fumbling my way though pull requests, CI tests and code review as there are all new for me! I appreciate your pointing out when/how components of this PR require improvement.

[coveralls]

Coverage increased (+0.01%) to 99.213% when pulling df6c2482ebf166d1988a43a7d3dbfea2fa9f3ceb on rthomson:master into 8607dd5accda2d6611148a410e5db59029e15d13 on AAbouZaid:devel.

[aabouzaid]

You actually are doing good :-) I'd suggest to have a boolean arg in _config, e.g. optional=Truel, so it passed that value to _get_value_by_path to ignore the error. That should be fine.

And in all cases, since there a new behavior has been introduced, it's fine to change the tests to fit that case.

[aabouzaid]

Actually now I'm thinking about another way to deal with config in general.

[aabouzaid]

@rthomson I did the fix to make any conf key optional, later I will find a better way to handle the config.

Could you please pull the branch optional_key and rebase it on yours then push it to merge.

Thanks.

[rthomson]

Could you please pull the branch optional_key and rebase it on yours then push it to merge.

I will take a look this week!

Thank you.

[coveralls]

Coverage increased (+0.03%) to 99.231% when pulling f58fa3a04fcf7877001bfe9cdb74af3607f05a71 on rthomson:master into 8607dd5accda2d6611148a410e5db59029e15d13 on AAbouZaid:devel.

[rthomson]

Pulling a new branch from upstream and rebasing against my changes, including manual merge conflict resolution, was an interesting learning experience!

I'm not sure but I think it might have worked...

[aabouzaid]

Well, now we have duplication of commits (the rebase should be on my last comment only). So you need to do something like that to fix it.

On your branch.

git checkout master
git checkout c646552
git push origin master -f

I think it's a nice experience in all cases :grin:

[rthomson]

Well, now we have duplication of commits (the rebase should be on my last comment only).

Oops.

I think it's a nice experience in all cases 😁

Absolutely! And thanks again for your patience.

I'll keep working at it.

[aabouzaid]

It has been merged :tada: It had some extra commits but I did rebase it on my side to avoid more toing and froing :-)

Thanks a lot for your effort @rthomson, keep going you are doing well :metal: