Skip to content

Comments

[atc/api] Enumerate Active Users#4096

Merged
taylorsilva merged 3 commits intomasterfrom
feature/4035-active-users
Aug 2, 2019
Merged

[atc/api] Enumerate Active Users#4096
taylorsilva merged 3 commits intomasterfrom
feature/4035-active-users

Conversation

@pivotal-bin-ju
Copy link
Contributor

@pivotal-bin-ju pivotal-bin-ju commented Jul 3, 2019

Fixes #4035

Looking to get some feedback on this code we have so far. Wondering about things like the error message we return here and just if we're on the right track so far with implementing this new endpoint.

CC @taylorsilva

@xtremerui
Copy link
Contributor

xtremerui commented Jul 4, 2019

https://ci.concourse-ci.org/builds/6450778#L5d1bd07e:1

Think you would need to bump the migration timestamp. Thx! @pivotal-bin-ju

jwntrs
jwntrs reviewed Jul 4, 2019
View reviewed changes
jwntrs
jwntrs reviewed Jul 4, 2019
View reviewed changes
jwntrs
jwntrs reviewed Jul 4, 2019
View reviewed changes
jwntrs
jwntrs reviewed Jul 4, 2019
View reviewed changes
jwntrs
jwntrs suggested changes Jul 4, 2019
View reviewed changes
Copy link
Contributor

@jwntrs jwntrs left a comment

Choose a reason for hiding this comment

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

Left some inline comments

@zoetian zoetian force-pushed the feature/4035-active-users branch from e5a00a3 to 05ae38c Compare July 8, 2019 21:43
@YoussB
Copy link
Contributor

YoussB commented Jul 11, 2019

@pivotal-jwinters, after @zoetian's update, do you have more comments are should we continue with the implementation of fly?

@YoussB YoussB force-pushed the feature/4035-active-users branch from b109916 to 3a9b7ba Compare July 24, 2019 18:31
@zoetian zoetian force-pushed the feature/4035-active-users branch 2 times, most recently from 1849d7a to 8f41ad9 Compare July 25, 2019 18:29
@YoussB YoussB marked this pull request as ready for review July 25, 2019 18:31
@YoussB
Copy link
Contributor

YoussB commented Jul 25, 2019

@pivotal-jwinters @xtremerui this should be ready for review now 👍

@YoussB YoussB requested a review from jwntrs July 25, 2019 18:32
jwntrs
jwntrs reviewed Jul 26, 2019
View reviewed changes
jwntrs
jwntrs previously requested changes Jul 26, 2019
View reviewed changes
Copy link
Contributor

@jwntrs jwntrs left a comment

Choose a reason for hiding this comment

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

Just that one small change

@zoetian zoetian force-pushed the feature/4035-active-users branch 2 times, most recently from 3c21e0f to 29f6b43 Compare July 26, 2019 18:45
@taylorsilva taylorsilva force-pushed the feature/4035-active-users branch 2 times, most recently from 11e5337 to d9e760c Compare July 30, 2019 18:36
@taylorsilva
Copy link
Member

taylorsilva commented Jul 30, 2019 •
edited
Loading

We have one unit test failing. See these links for details/stack traces:

but it's only failing in CI. It doesn't fail locally. Thought it might be an issue with different ginkgo versions between ci and local but that was a dead-end. (ci and local are both using ginkgo v1.8.0)

Figured it out; had to do with location changing after unmarshaling json encoded time.Time.

Expected
          <string>: UTC
      to equal
          <string>: Local

When we create a time.Time object it has a default location value of Local. We then JSON encode it in our test and then unmarshal it, this results in the location changing to UTC. This is only happening on the linux system though. Possibly a bug with Go on linux, where unmarshaling time from json always sets the location to UTC?

Fixed it by setting location to UTC right from the start. Passes on both systems now.

@taylorsilva taylorsilva force-pushed the feature/4035-active-users branch 2 times, most recently from a700f3b to 1e31bea Compare July 31, 2019 14:48
@taylorsilva
Copy link
Member

taylorsilva commented Jul 31, 2019

Got all the tests passing!

@pivotal-jwinters @xtremerui one last review and then merge please 😄

@taylorsilva taylorsilva requested a review from jwntrs July 31, 2019 15:19
xtremerui
xtremerui reviewed Jul 31, 2019
View reviewed changes
@taylorsilva taylorsilva force-pushed the feature/4035-active-users branch from 9cc65c8 to fa2026e Compare July 31, 2019 21:47
@pivotal-bin-ju @YoussB @taylorsilva
[fly] [api] Enumerating Active Users:
c75d699 
- Adds `users` table up and down migrations.
- Adds ATC functionality to write or update the user in the db on a
successful login.
- Adds ATC api functionality under `/api/v1/users`.
- Adds `fly users` command.
- Adds unit tests in place.

Signed-off-by: Bin Ju <[email protected]>
Co-authored-by: Bishoy Youssef <[email protected]>
Co-authored-by: Ciro S. Costa <[email protected]>
Co-authored-by: Taylor Silva <[email protected]>
Co-authored-by: Zoe Tian <[email protected]>
Signed-off-by: Zoe Tian <[email protected]>
Zoe Tian and others added 2 commits July 31, 2019 17:52
@YoussB @taylorsilva
Adds sub as a column to the users table
29bb883 
- also removed it from the fly command.

Signed-off-by: Zoe Tian <[email protected]>
Co-authored-by: Bishoy Youssef <[email protected]>
Signed-off-by: Bin Ju <[email protected]>
Co-authored-by: Taylor Silva <[email protected]>
@taylorsilva
Change LastLogin type to int64
fd87e21 
Signed-off-by: Taylor Silva <[email protected]>
@taylorsilva taylorsilva force-pushed the feature/4035-active-users branch from fa2026e to fd87e21 Compare July 31, 2019 21:53
@taylorsilva taylorsilva requested a review from xtremerui July 31, 2019 21:53
@taylorsilva
Copy link
Member

taylorsilva commented Aug 1, 2019

@pivotal-jwinters @xtremerui switched the LastLogin type from time.Time to int64. Please review again

@taylorsilva taylorsilva dismissed jwntrs’s stale review August 2, 2019 21:11

Resolved, lost history from force pushing though.

@taylorsilva taylorsilva merged commit 52238f3 into master Aug 2, 2019
@taylorsilva taylorsilva deleted the feature/4035-active-users branch August 2, 2019 21:12
jamieklassen pushed a commit to concourse/docs that referenced this pull request Aug 29, 2019
document fly active-users
81de58b 
concourse/concourse#4096

Signed-off-by: James Thomson <[email protected]>
Co-authored-by: Jamie Klassen <[email protected]>
@jamieklassen jamieklassen added this to the v5.5.0 milestone Aug 29, 2019
@jamieklassen jamieklassen added the release/documented Documentation and release notes have been updated. label Aug 29, 2019
jamieklassen pushed a commit to concourse/docs that referenced this pull request Aug 29, 2019
add active-users to permissions matrix
3022d2c 
concourse/concourse#4096

Signed-off-by: Jamie Klassen <[email protected]>
Co-authored-by: James Thomson <[email protected]>
jamieklassen pushed a commit that referenced this pull request Aug 29, 2019
document enumerate users
e6e7b0b 
#4096

Signed-off-by: Jamie Klassen <[email protected]>
Co-authored-by: James Thomson <[email protected]>
matthewpereira pushed a commit that referenced this pull request Sep 5, 2019
document enumerate users
99c944f 
#4096

Signed-off-by: Jamie Klassen <[email protected]>
Co-authored-by: James Thomson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taylorsilva taylorsilva taylorsilva left review comments

@xtremerui xtremerui xtremerui approved these changes

@jwntrs jwntrs Awaiting requested review from jwntrs

+1 more reviewer

@zoetian zoetian zoetian left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release/documented Documentation and release notes have been updated.

Projects

None yet

Milestone

v5.5.0

Development

Successfully merging this pull request may close these issues.

Ability to enumerate active users

7 participants

@pivotal-bin-ju @xtremerui @YoussB @taylorsilva @jwntrs @zoetian @jamieklassen