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

Manage a token name only once #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mohabusama
Copy link

Manage a token name only once. This avoids resetting already valid token for the same token name.

  • some styling and tests

Signed-off-by: Mohab Usama [email protected]

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4488cce on mohabusama:manage-token-once into fc6cedf on zalando-stups:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4488cce on mohabusama:manage-token-once into fc6cedf on zalando-stups:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4488cce on mohabusama:manage-token-once into fc6cedf on zalando-stups:master.

@mohabusama
Copy link
Author

@hjacobs could you please take a look?

@hjacobs
Copy link
Contributor

hjacobs commented Dec 18, 2018

LGTM, but there is the theoretical breaking change for situations where multiple manage calls were made for the same token name: the latest config won before this PR, now the first call to manage wins. Probably just a theoretical case, but still a noteworthy change in behaviour.

@mohabusama
Copy link
Author

Yeah, thought about it!
Was thinking about allow resetting in case the scopes changed, but that would involve locking (threading or multiprocessing?) to be more or less accurate. I felt it would get complex compared to a rather more straightforward/expected usage: "call this once for each token".

@hjacobs
Copy link
Contributor

hjacobs commented Dec 19, 2018

👍

1 similar comment
@mohabusama
Copy link
Author

👍

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.

3 participants