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 task for creating an API token for the superuser #128

Merged
merged 13 commits into from
May 22, 2021
Merged

Add task for creating an API token for the superuser #128

merged 13 commits into from
May 22, 2021

Conversation

florianow
Copy link
Contributor

hey,
i create a new default for deploy a api token for the superuser. we needed this for our project and let you know about this :)
thanks!

Copy link
Owner

@lae lae left a comment

Choose a reason for hiding this comment

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

Good style overall. Some additional things apart from the review comments:

  • Please add documentation to the README.adoc for the new role variable explaining what it does.
  • create_token might be a more appropriate name than 'token_enabled', since that's what's happening here.
  • Preferably, the role variable should also be configured in tests/group_vars as well so that CI will test it.

molecule/default/molecule.yml Outdated Show resolved Hide resolved
vars/main.yml Outdated Show resolved Hide resolved
@lae lae changed the title create a superuser token bool Add task for creating an API token for the superuser Mar 27, 2021
@florianow
Copy link
Contributor Author

in our Environment we deploy the netbox with your role and after it we discover all of our infrastructure (that´s the reason for the token). We already use LDAP too. I changed it to an bool trigger and an or condition. I hope that's also fine for you.

@fiskhest
Copy link

This'd be a great addition! Could we get a new review @lae ?

Copy link
Owner

@lae lae left a comment

Choose a reason for hiding this comment

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

sorry, missed the followup comment. New review with a question.

.cache/roles/Musee Ullah.netbox Outdated Show resolved Hide resolved
README.adoc Outdated Show resolved Hide resolved
README.adoc Outdated Show resolved Hide resolved
tests/group_vars/netbox Outdated Show resolved Hide resolved
tasks/deploy_netbox.yml Outdated Show resolved Hide resolved
tasks/deploy_netbox.yml Outdated Show resolved Hide resolved
tests/group_vars/netbox Outdated Show resolved Hide resolved
@lae lae merged commit 00c622b into lae:master May 22, 2021
@lae
Copy link
Owner

lae commented May 22, 2021

Changes merged, but squashed into one commit due to the noisy commit history.

This may pose an issue when trying to pull changes to your fork as you were working directly on your main branch (master) because the histories are different. There are various ways of dealing with this, but the easiest might just be deleting your fork and making a new fork. To avoid this in the future, consider creating a new temporary branch (we tend to call them "feature branches" or something similar, but they're technically all the same), making changes to that branch and then creating a PR off of that branch.

@florianow
Copy link
Contributor Author

HI, Thanks for the Information. I will do it like this in the future.

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

Successfully merging this pull request may close these issues.

None yet

3 participants