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.

Sign up for GitHub

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

Jump to bottom

Gabbi/add token verify endpoint #59

Merged
merged 1 commit into from
Nov 8, 2019
Merged

Gabbi/add token verify endpoint #59

merged 1 commit into from
Nov 8, 2019

Conversation

gabbifish
Copy link
Contributor

This PR adds an endpoint for verifying that an API token is active and legitimate.

PLEASE REVIEW THIS ONLY AFTER #58 HAS BEEN MERGED.

@gabbifish gabbifish mentioned this pull request Nov 5, 2019
Merged
ashleymichal
ashleymichal suggested changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@ashleymichal ashleymichal left a comment

Choose a reason for hiding this comment

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

similar to the way we do other endpoints, consider a dir here for consistency (data structures in mod.rs, named files for endpoints like get_token_status.rs. there's only one endpoint though, so i won't block on this unless others also feel strongly.

src/endpoints/user.rs Outdated
permissions: Vec<String>,
roles: Vec<String>,
}

#[derive(Deserialize, Serialize, Debug, Clone, PartialEq, Eq)]
pub struct UserDetails {
Copy link
Contributor

Choose a reason for hiding this comment

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

idk if a stutter is objectionable to others, but likely i would reference this module from above, so user::Details. Same w/ endpoint structs and names, so user::TokenStatus rather than user::UserTokenStatus, etc..

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, I really like how Go encourages you to write e.g. stream.Buffered instead of stream.BufferedStream. It's nice but not essential.

Copy link
Contributor

Choose a reason for hiding this comment

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

renaming in a public api is hard, so i'd rather block today 😀

@adamchalmers
Copy link
Contributor

similar to the way we do other endpoints, consider a dir here for consistency (data structures in mod.rs, named files for endpoints like get_token_status.rs. there's only one endpoint though, so i won't block on this unless others also feel strongly.

Agree that it's nice but not necessary when you only have one file, approved.

@ashleymichal ashleymichal force-pushed the gabbi/add-token-verify-endpoint branch from 483b44b to 52b372e Compare November 6, 2019 21:24
@ashleymichal ashleymichal mentioned this pull request Nov 6, 2019
Closed
@ashleymichal
Copy link
Contributor

added suggestions to a pr but i'm not going to block on them, this looks good.

@gabbifish gabbifish merged commit 5cc2d9f into master Nov 8, 2019
@delete-merged-branch delete-merged-branch bot deleted the gabbi/add-token-verify-endpoint branch November 8, 2019 16:24
gabbifish added a commit that referenced this pull request Nov 10, 2019
@gabbifish
add token verify endpoint (#59)
e789563
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamchalmers adamchalmers adamchalmers approved these changes

@ashleymichal ashleymichal ashleymichal approved these changes

@sssilver sssilver Awaiting requested review from sssilver

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gabbifish @adamchalmers @ashleymichal