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 the identity plugin #1384

Merged
merged 1 commit into from Sep 20, 2019
Merged

Add the identity plugin #1384

merged 1 commit into from Sep 20, 2019

Conversation

@decentralion
Copy link
Member

decentralion commented Sep 18, 2019

This commit adds the new SourceCred identity plugin. As described in the
README.md file:

This folder contains the Identity plugin. Unlike most other plugins, the
Identity plugin does not add any new contributions to the graph. Instead, it
allows collapsing different user accounts together into a shared 'identity'
node.

To see why this is valuable, imagine that a contributor has an account on both
GitHub and Discourse (potentially with a different username on each service).
We would like to combine these two identities together, so that we can
represent that user's combined cred properly. The Identity plugin enables this.

Specifically, the instance maintainer can provide a (locally unique) username
for the user, along with a list of aliases the user is known by, e.g.
github/username and discourse/other_username. The aliases are simple string
representations, that are intended to be easy to maintain by hand in a
configuration file. Then, the identity plugin will provide a list of
NodeContractions that can be used by Graph.contractNodes to combine the
user identities as described.

The plugin is broken up into a few submoudles:

  • declaration.js provides the PluginDeclaration. It has a single node
    type (the identity node).
  • nodes.js allows constructing identity nodes, and does some
    validation on the username used to make the node.
  • alias.js implements the logic for parsing aliases like
    "github/decentralion" or "discourse/s_ben" into a node address.
  • nodeContractions.js declares the Identity type (a username and a
    list of aliases) and provides logic for turning a list of Identities
    into a list of NodeContractions, suitable for use in
    Graph.contractNodes.

The plugin is not yet integrated; that will come in a followon commit.

Test plan: Unit tests added; yarn test passes.

@decentralion decentralion requested review from Beanow and wchargin Sep 18, 2019
decentralion added a commit that referenced this pull request Sep 18, 2019
This commit integrates the identity plugin, which was created in #1384.
It does this by adding explicit identity fields to the project
configuration, which are then applied when loading the graph in
`api/load.js`.

The actual integration is quite straightforward.

Test plan: The underlying logic is thoroughly tested; I added one new
test case to verify that it is integrated properly. Since the project
compat has changed, I've updated all the snapshots. Prior to merging
this PR, I will produce one "integration test", using this code to do
identity resolution for a real project (i.e. on the SourceCred instance
itself).
decentralion added a commit that referenced this pull request Sep 18, 2019
This commit integrates the identity plugin, which was created in #1384.
It does this by adding explicit identity fields to the project
configuration, which are then applied when loading the graph in
`api/load.js`.

The actual integration is quite straightforward.

Test plan: The underlying logic is thoroughly tested; I added one new
test case to verify that it is integrated properly. Since the project
compat has changed, I've updated all the snapshots. Prior to merging
this PR, I will produce one "integration test", using this code to do
identity resolution for a real project (i.e. on the SourceCred instance
itself).
export function resolveAlias(
alias: Alias,
discourseUrl: string | null
): NodeAddressT {

This comment has been minimized.

Copy link
@Beanow

Beanow Sep 18, 2019

Member

I believe the error handling may be significantly simplified with a regex.
For example /^(github|discourse)\/@?(\w+)$/.
If explicit errors about unknown types are desired, removing the explicit options works too.
/^(\w+)\/@?(\w+)$/

This comment has been minimized.

Copy link
@decentralion

decentralion Sep 20, 2019

Author Member

Re-wrote it to use a regex per your suggestion.


export function resolveAlias(
alias: Alias,
discourseUrl: string | null

This comment has been minimized.

Copy link
@Beanow

Beanow Sep 18, 2019

Member

This API (along with the githubAddress and discourseAddress dependencies) isn't very extensible or neutral as a plugin. Instead, you might provide a map like Map<string, function> where string is your type/prefix, the function has a UsernameT => NodeAddressT signature.

This comment has been minimized.

Copy link
@Beanow

Beanow Sep 18, 2019

Member

You might even consider turning this into a class or factory, to separate providing the functions from resolving an alias.

// factory style
const createAliasResolver = (mapping: Map<string, function>) => (alias: Alias) => { ... }

// OO style
class AliasResolver {
 ...
 constructor (mapping: Map<string, function>) { ... }
 resolve (alias: Alias) { ... }
}

This comment has been minimized.

Copy link
@decentralion

decentralion Sep 19, 2019

Author Member

This is a very reasonable suggestion. However, I'm not going to do so in this PR. Right now, the Discourse and GitHub plugins are explicitly privileged all over the place (e.g. the project data format explicitly lists repo ids and a discourse server, the CLI explicitly looks for these keys, etc). I don't think there's much benefit to modularizing the plugin system piecemeal; instead, I'm planning to do a big refactor later, when we start to see external interest in adding plugins. At that point we can ensure the entire system has a clean and coherent design.

@decentralion decentralion force-pushed the identity-plugin branch from c9e798f to c76a27d Sep 19, 2019
decentralion added a commit that referenced this pull request Sep 19, 2019
This commit integrates the identity plugin, which was created in #1384.
It does this by adding explicit identity fields to the project
configuration, which are then applied when loading the graph in
`api/load.js`.

The actual integration is quite straightforward.

Test plan: The underlying logic is thoroughly tested; I added one new
test case to verify that it is integrated properly. Since the project
compat has changed, I've updated all the snapshots. Prior to merging
this PR, I will produce one "integration test", using this code to do
identity resolution for a real project (i.e. on the SourceCred instance
itself).
@decentralion decentralion force-pushed the identity-plugin branch from c76a27d to 9379815 Sep 20, 2019
This commit adds the new SourceCred identity plugin. As described in the
README.md file:

This folder contains the Identity plugin. Unlike most other plugins, the
Identity plugin does not add any new contributions to the graph. Instead, it
allows collapsing different user accounts together into a shared 'identity'
node.

To see why this is valuable, imagine that a contributor has an account on both
GitHub and Discourse (potentially with a different username on each service).
We would like to combine these two identities together, so that we can
represent that user's combined cred properly. The Identity plugin enables this.

Specifically, the instance maintainer can provide a (locally unique) username
for the user, along with a list of aliases the user is known by, e.g.
`github/username` and `discourse/other_username`. The aliases are simple string
representations, that are intended to be easy to maintain by hand in a
configuration file. Then, the identity plugin will provide a list of
`NodeContraction`s that can be used by `Graph.contractNodes` to combine the
user identities as described.

The plugin is broken up into a few submoudles:
- `declaration.js` provides the PluginDeclaration. It has a single node
type (the identity node).
- `identity.js` declares the `Identity` type (a username and list of
aliases), allows constructing identity nodes, and does some validation
on the identity username.
- `alias.js` implements the logic for parsing aliases like
"github/decentralion" or "discourse/s_ben" into a node address.
- `nodeContractions.js` provides logic for turning a list of Identities
into a list of NodeContractions, suitable for use in
`Graph.contractNodes`.

The plugin is not yet integrated; that will come in a followon commit.

Test plan: Unit tests added; `yarn test` passes.
@decentralion decentralion force-pushed the identity-plugin branch from 9379815 to 1770edb Sep 20, 2019
decentralion added a commit that referenced this pull request Sep 20, 2019
This commit integrates the identity plugin, which was created in #1384.
It does this by adding explicit identity fields to the project
configuration, which are then applied when loading the graph in
`api/load.js`.

The actual integration is quite straightforward.

Test plan: The underlying logic is thoroughly tested; I added one new
test case to verify that it is integrated properly. Since the project
compat has changed, I've updated all the snapshots. Prior to merging
this PR, I will produce one "integration test", using this code to do
identity resolution for a real project (i.e. on the SourceCred instance
itself).
@decentralion decentralion merged commit 9a9f211 into master Sep 20, 2019
2 checks passed
2 checks passed
ci/circleci: publish-1 Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
decentralion added a commit that referenced this pull request Sep 20, 2019
This commit integrates the identity plugin, which was created in #1384.
It does this by adding explicit identity fields to the project
configuration, which are then applied when loading the graph in
`api/load.js`.

The actual integration is quite straightforward.

Test plan: The underlying logic is thoroughly tested; I added one new
test case to verify that it is integrated properly. Since the project
compat has changed, I've updated all the snapshots. Prior to merging
this PR, I will produce one "integration test", using this code to do
identity resolution for a real project (i.e. on the SourceCred instance
itself).
decentralion added a commit that referenced this pull request Sep 20, 2019
This commit integrates the identity plugin, which was created in #1384.
It does this by adding explicit identity fields to the project
configuration, which are then applied when loading the graph in
`api/load.js`.

The actual integration is quite straightforward.

Test plan: The underlying logic is thoroughly tested; I added one new
test case to verify that it is integrated properly. Since the project
compat has changed, I've updated all the snapshots. Prior to merging
this PR, I will produce one "integration test", using this code to do
identity resolution for a real project (i.e. on the SourceCred instance
itself).
decentralion added a commit that referenced this pull request Sep 20, 2019
This commit integrates the identity plugin, which was created in #1384.
It does this by adding explicit identity fields to the project
configuration, which are then applied when loading the graph in
`api/load.js`.

The actual integration is quite straightforward.

Test plan: The underlying logic is thoroughly tested; I added one new
test case to verify that it is integrated properly. Since the project
compat has changed, I've updated all the snapshots. Prior to merging
this PR, I will produce one "integration test", using this code to do
identity resolution for a real project (i.e. on the SourceCred instance
itself).
@decentralion decentralion deleted the identity-plugin branch Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.