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

Integrate the identity plugin #1385

Merged
merged 1 commit into from Sep 20, 2019
Merged

Integrate the identity plugin #1385

merged 1 commit into from Sep 20, 2019

Conversation

@decentralion
Copy link
Member

decentralion commented 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 decentralion requested review from Beanow and wchargin Sep 18, 2019
@decentralion decentralion force-pushed the identity-integration branch from 70e44a6 to 505dd15 Sep 18, 2019
@decentralion

This comment has been minimized.

Copy link
Member Author

decentralion commented Sep 19, 2019

@Beanow / @vsoch it looks like this commit is consistently failing the docker publish test, see failure.

Can you take a look into what's going wrong?

The message is:

Build-agent version 1.0.15410-75b89bda (2019-09-13T15:43:07+0000)
Creating a dedicated VM with sourcecred/sourcecred image
failed to create host: Image sourcecred/sourcecred is not supported

failed to create host: Image sourcecred/sourcecred is not supported
@Beanow

This comment has been minimized.

Copy link
Member

Beanow commented Sep 19, 2019

It actually seems like a flake on CircleCI's side to me. Recent builds of the same command docker/publish-1 have worked. And none of our code is executed yet. Just Circle allocating a VM for the job.

@decentralion decentralion force-pushed the identity-plugin branch from c9e798f to c76a27d Sep 19, 2019
@decentralion decentralion force-pushed the identity-integration branch from 505dd15 to 3c356ee Sep 19, 2019
@Beanow

This comment has been minimized.

Copy link
Member

Beanow commented Sep 19, 2019

Actually it is a real issue, possibly a bug with the docker orb we're using.
On other recent runs, like https://circleci.com/gh/sourcecred/sourcecred/2227#config/containers/0 it expands the configuration into:

  docker/publish-1:
    machine:
      docker_layer_caching: false
      image: ubuntu-1604:201903-01

While this run it expands into:

  docker/publish-1:
    machine:
      docker_layer_caching: false
      image: sourcecred/sourcecred
Beanow added a commit to Beanow/sourcecred that referenced this pull request Sep 19, 2019
@Beanow

This comment has been minimized.

Copy link
Member

Beanow commented Sep 19, 2019

I've submitted a bugreport of this happening here CircleCI-Public/docker-orb#19

And have a mitigation here: https://github.com/sourcecred/sourcecred/tree/pr/anti-flake-docker-orb

If the issue persists, you may try cherry picking that commit / merging it separately.

@vsoch

This comment has been minimized.

Copy link
Contributor

vsoch commented Sep 19, 2019

Where do you see it's failing publish? I see an error at yarn test --ci but maybe I missed it?

There was another weird CircleCI error that I saw yesterday, let me see if I can find it:

Build-agent version 1.0.15410-75b89bda (2019-09-13T15:43:07+0000)
Creating a dedicated VM with nushell/nu-base image
failed to create host: Image nushell/nu-base is not supported

failed to create host: Image nushell/nu-base is not supported

In this case, also using the Docker Publish orb, and it didn't even get to set up the environment.

@Beanow

This comment has been minimized.

Copy link
Member

Beanow commented Sep 19, 2019

It's the same error as you're getting @vsoch and what I reported at CircleCI-Public/docker-orb#19

If you go to the "Configuration" tab of such a failed build, you'll probably see the nushell/nu-base image got incorrectly passed onto the machine executor VM image. Which isn't right because this should be a VM image for a docker host.

@vsoch

This comment has been minimized.

Copy link
Contributor

vsoch commented Sep 19, 2019

That's so strange! There haven't been changes to the Orb itself (that repo) since July 23rd, so it must be something internal at Circle. I'm not sure how responsive they are there, because I opened a PR on that exact repo 9 days ago and it's been 🏏 .

@decentralion decentralion force-pushed the identity-plugin branch 2 times, most recently from 9379815 to 1770edb Sep 20, 2019
@decentralion decentralion force-pushed the identity-integration branch from 3c356ee to 2b606cb Sep 20, 2019
@decentralion decentralion changed the base branch from identity-plugin to master Sep 20, 2019
@decentralion decentralion force-pushed the identity-integration branch from 2b606cb to 1576e54 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 force-pushed the identity-integration branch from 1576e54 to aae2ed0 Sep 20, 2019
@decentralion decentralion merged commit 54ece53 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 decentralion deleted the identity-integration branch Sep 20, 2019
@decentralion

This comment has been minimized.

Copy link
Member Author

decentralion commented Sep 20, 2019

@vsoch @Beanow it wound working fine (both builds passed). not sure what changed.

good example here of references not corresponding super well with cred for the PR, btw :P

Beanow added a commit that referenced this pull request Dec 14, 2019
Creation of new Project instances is spread out across the code.
So whenever there's a change in it's format, the PR is cluttered
with adding a logical default value in many places.

For example #1385 adds many `identities: [],` lines.
A similar situation would happen with the planned Initiatives
plugin, adding many `initiatives: null,` lines.

Using this function we can manage what default values to add
from a central place. Avoiding noise and code churn.
Beanow added a commit that referenced this pull request Dec 14, 2019
Creation of new Project instances is spread out across the code.
So whenever there's a change in it's format, the PR is cluttered
with adding a logical default value in many places. It means
our default values might be inconsistent as well.

For example #1385 adds many `identities: [],` lines.
A similar situation would happen with the planned Initiatives
plugin, adding many `initiatives: null,` lines.

Using this function we can manage what default values to add
from a central place. Avoiding noise and code churn.
decentralion added a commit that referenced this pull request Dec 30, 2019
Creation of new Project instances is spread out across the code.
So whenever there's a change in it's format, the PR is cluttered
with adding a logical default value in many places. It means
our default values might be inconsistent as well.

For example #1385 adds many `identities: [],` lines.
A similar situation would happen with the planned Initiatives
plugin, adding many `initiatives: null,` lines.

Using this function we can manage what default values to add
from a central place. Avoiding noise and code churn.
Beanow added a commit that referenced this pull request Jan 3, 2020
Creation of new Project instances is spread out across the code.
So whenever there's a change in it's format, the PR is cluttered
with adding a logical default value in many places. It means
our default values might be inconsistent as well.

For example #1385 adds many `identities: [],` lines.
A similar situation would happen with the planned Initiatives
plugin, adding many `initiatives: null,` lines.

Using this function we can manage what default values to add
from a central place. Avoiding noise and code churn.
Beanow added a commit that referenced this pull request Jan 3, 2020
Creation of new Project instances is spread out across the code.
So whenever there's a change in it's format, the PR is cluttered
with adding a logical default value in many places. It means
our default values might be inconsistent as well.

For example #1385 adds many `identities: [],` lines.
A similar situation would happen with the planned Initiatives
plugin, adding many `initiatives: null,` lines.

Using this function we can manage what default values to add
from a central place. Avoiding noise and code churn.
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

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