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

Better user-key documentation directions #171

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

FontouraAbreu
Copy link

Description

The following files where modified:

  • README.md
  • docs/models/userkey.md

The following files where created:

  • assets/first-user-key.png

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Other (please describe):

Test Cases

  • Test cases are covered for this PR.
  • This PR does not require test cases (please explain): Documentation update only

Additional Information

I made the changes based on a difficulty I had when first-time configuring secrets. I did find the proper documentation by docs/models/userkey.md but nothing was pointing to it at the README.md and nothing was said about the need to click on the save button for properly saving my new generated keys.

Copy link
Contributor

@abhi1693 abhi1693 left a comment

Choose a reason for hiding this comment

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

For now, I have some minor nit picks.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@FontouraAbreu
Copy link
Author

Sure thing!

Will make the modifications and also add some more usage examples using pynetbox.

@FontouraAbreu
Copy link
Author

I've made the changes as well as added a whole Pynetbox examples section.
Every documentation was merged into the main Readme.md file.

I fixed an error where the API endpoints where <netbox_url>/api/plugins/secrets/secrets/secrets/<any> instead of <netbox_url>/api/plugins/secrets/secrets/<any>. Also changed an incorrect "Netbox Admin UI" path that was removed from netbox in a previous update to a sidebar menu.

I changed some text in order to make the documentation a bit more readable.

Please check it out, specially the Pynetbox section.

Obs.: Not every pynetbox example has been tested, in order to do these i'd like first your opinion on the documentation arrangement :)

"plaintext": "foobar",
}

nb.plugins.secrets.secrets.create(data=secret, session=session)
Copy link

Choose a reason for hiding this comment

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

Sorry for stepping in but I found an error calling the pynetbox API here:

Using this line from the example, I got

ynetbox.core.query.RequestError: The request failed with code 400 Bad Request: {'assigned_object_type': ['This field is required.'], 'assigned_object_id': ['This field is required.'], 'role': ['This field is required.'], 'plaintext': ['This field is required.']}

After trying around a bit, I got it working by using this call instead nb.plugins.secrets.secrets.create(secret) because create() is defined as create(*args, **kwargs), taking the properties of the objects to be created directly, or a list of them as dicts. See https://pynetbox.readthedocs.io/en/latest/endpoint.html#pynetbox.core.endpoint.Endpoint.create

Nevertheless this documentation enhancement is awesome.

Copy link
Author

Choose a reason for hiding this comment

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

Good one!

I've not tested everything, just put on what I thought would work. I'll take some time to test everything up and update the pull request.

Thanks for the correction!

@abhi1693
Copy link
Contributor

Before releasing the plugin for v4.2, I'll go through this doc and make sure it's good to merge. I'd rather have this change with the newer release than add to existing ones.

@FontouraAbreu
Copy link
Author

That's perfect!

I'm aiming to upgrade to v4.2 and netbox-secrets is the only plugin left to enable the upgrade.


```json
{
"private_key": "<private key>"
Copy link
Contributor

Choose a reason for hiding this comment

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

The API payload also requires the user_keys field to activate the user keys. This field should be a list in the following format:

{
  "user_keys": [1, 2]
}

Here, the integers represent the user IDs.

Please ensure to test the API thoroughly when writing its documentation to confirm proper functionality and accuracy.

Copy link
Author

Choose a reason for hiding this comment

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

okay


```json
{
"private_key": "<private key>"
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that it doesn't mention the optional preserve_key field in the payload. This field allows users to reuse an existing session key instead of creating a new one.

"plaintext": "foobar",
}

nb.plugins.secrets.secrets.update(id=1, data=secret, session=session)
Copy link
Contributor

Choose a reason for hiding this comment

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

The update method requires the id to be included in the payload rather than as a parameter. Here is the correct payload example:

secret = {
    "id": 1,
    "assigned_object_type": "dcim.device",
    "assigned_object_id": 35,
    "role": 1,
    "name": "admin",
    "plaintext": "foobar",
}
nb.plugins.secrets.secrets.update([secret])

Make sure to pass the payload in this format when updating an instance.

If we want to update a single instance, we first need to fetch the instance and then update it like this:

x = nb.dcim.devices.get(1)
x.update({
    "name": "test-switch2",
    "serial": "ABC321",
})

Copy link
Contributor

@kprince28 kprince28 left a comment

Choose a reason for hiding this comment

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

Could you please thoroughly test the API before writing the documentation? I've noticed some mistakes in the API docs, and proper testing would help ensure accuracy and clarity.

@FontouraAbreu
Copy link
Author

sure thing. I will aim to add a tests directory in order to enable unit tests for these API calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants