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

Add configuration parsing #20

Merged
merged 4 commits into from
Aug 31, 2023
Merged

Add configuration parsing #20

merged 4 commits into from
Aug 31, 2023

Conversation

ByteOtter
Copy link
Member

@ByteOtter ByteOtter commented Aug 25, 2023

What does this PR change?

There are at least two variables that users need to be able to configure when using netbox-sync:

  • The URI of their netbox API endpoints
  • The authentication token for the API

This PR implements the first stage of adding the ability to configure the program by using a .nbs-config.toml file.

This file will be located in etc/opt/.nb-config.toml and at least consist of the following:

[netbox]
netbox_auth_token = "<authentication token>"
netbox_uri = "<http uri to netbox instance>"

Tick the applicable box:

  • Add new feature
  • Security changes
  • Tests
  • Documentation changed

  • General Maintenance

Links

Tracks: #21

  • DONE

Documentation

  • Documentation provided by buildable docstrings.

  • DONE

@ByteOtter ByteOtter force-pushed the dev/add-configuration branch 2 times, most recently from bf381e2 to c178f69 Compare August 25, 2023 15:01
@ByteOtter ByteOtter self-assigned this Aug 25, 2023
@ByteOtter ByteOtter force-pushed the dev/add-configuration branch 4 times, most recently from 50bffa5 to 6b79a73 Compare August 28, 2023 15:56
@ByteOtter ByteOtter marked this pull request as ready for review August 31, 2023 08:14
@ByteOtter ByteOtter mentioned this pull request Aug 31, 2023
3 tasks
@ByteOtter
Copy link
Member Author

ByteOtter commented Aug 31, 2023

This is done as far as possible. It will be touched again for work on #22 though as taking config parameters from the cli is mandatory if the config file is empty.

Also, as this feature mainly relies on file system operations and other parts of the program, I do not think that writing unittests is very advisable.
We should cover this with integrations tests as laid out in #23 .

@ByteOtter ByteOtter force-pushed the dev/add-configuration branch from 3362d84 to 3f3d82d Compare August 31, 2023 08:22
@ByteOtter ByteOtter requested a review from SchoolGuy August 31, 2023 08:23
Copy link
Collaborator

@SchoolGuy SchoolGuy left a comment

Choose a reason for hiding this comment

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

I would have implemented it differently but the code you wrote is fine and as such this PR is LGTM.

@ByteOtter
Copy link
Member Author

I would have implemented it differently but the code you wrote is fine and as such this PR is LGTM.

Thanks for your review!
I'd be happy to hear your thoughts about the implementations when you have the time :)

@ByteOtter ByteOtter merged commit 602c842 into main Aug 31, 2023
@ByteOtter ByteOtter deleted the dev/add-configuration branch August 31, 2023 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants