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

A new execution module: PeeringDB #48688

Merged
merged 4 commits into from Aug 1, 2018

Conversation

mirceaulinic
Copy link
Contributor

@mirceaulinic mirceaulinic commented Jul 20, 2018

What does this PR do?

Adds another execution module, for the basic interaction with the PeeringDB API. This is useful to gather data about other networks you can potentially peer with, and automatically establish BGP sessions, e.g., given just a specific AS number , the rest of the information (i.e., IP addresses, places where the remote network is available, etc.) is retrieved from PeeringDB, and the session configuration is automated with minimum to no effort (typing manually IP addresses is both tedious and error prone).

@mirceaulinic mirceaulinic changed the title A new execution module: Peeringdb A new execution module: PeeringDB Jul 20, 2018
'result': True,
'out': None
}
res = salt.utils.http.query(url,
Copy link
Contributor

Choose a reason for hiding this comment

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

This will default to HTTP. Should we prefer HTTPS given that we're passing credentials in the clear here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely. Is there an option to salt.utils.http.query for that? I've never used it, I'm afraid. Though, the base URL is PEERINGDB_URL = 'https://www.peeringdb.com/api' and I have assumed that it would use HTTPS therefore. Let me know and I'll update, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! You are right. Yup, we're good here. False alarm on my part.

@rallytime
Copy link
Contributor

This is causing a related test to fail: https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py2/job/PR-48688/2/

@mirceaulinic Can you take a look?

@mirceaulinic
Copy link
Contributor Author

mirceaulinic commented Jul 23, 2018

@rallytime it seem to me that is complain about the clean_kwargs utility function (https://github.com/saltstack/salt/blob/develop/salt/utils/args.py#L30) which doesn't have an example.
I believe this new test will complain again in the future, happy to add an example in the docstring if you want.

@rallytime
Copy link
Contributor

@mirceaulinic That'd be great! Thanks!

@mirceaulinic
Copy link
Contributor Author

I pushed dd10817 - please let me know @rallytime if this don't suffice to pass the tests.

@rallytime
Copy link
Contributor

@mirceaulinic That test is picky and is specifically looking for CLI Example. I updated your PR here to get them running again.

Thanks again for his and all of your awesome work!

@rallytime
Copy link
Contributor

Hrm, I read the test failure incorrectly. Now it says:

No docstring:
  - peeringdb.clean_kwargs

@mirceaulinic
Copy link
Contributor Author

HI @rallytime - yes, clean_kwargs is imported from salt.utils.args. Bizarrely, it is imported in the exact same way in several other modules, but the tests didn't complain. Is there anything I can do? Thanks!

@mirceaulinic
Copy link
Contributor Author

It looks like a rebase helped, the lint check is passing now...

@rallytime
Copy link
Contributor

@mirceaulinic Hrm...still not passing. We just need to add that function to the list of functions to ignore. Can you add that here?

@mirceaulinic
Copy link
Contributor Author

I pushed fc000b8 let's see if that helps.

@rallytime rallytime added the ZRELEASED - Fluorine reitred label label Jul 31, 2018
@rallytime rallytime merged commit 48bcef9 into saltstack:develop Aug 1, 2018
@mirceaulinic mirceaulinic deleted the peeringdb-mod branch August 9, 2018 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants