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

adding models to enterprise owner work #4

Conversation

danbrauer
Copy link

@danbrauer danbrauer commented Nov 8, 2024

This is a local PR in our fork only. It is a space to work off our WIP gh_identity_enterprise_owners branch/PR where we can refactor that work to create and use Cartography schemas. Once it works here, locally, I will merge it into that branch, which then will be visible to Cartography since that branch is now in a PR they can see.

Current state

  • I found what seems like a problem with this schema approach, where GitHubOrganization nodes are now not created at all, by the orm, whereas before with the bespoke Cypher loading, they were. I asked Alex (cartography maintainer) about that.
  • there may be other issues, but it was hard to keep troubleshooting given the above
    *WIP

@danbrauer danbrauer changed the title initial poking around incomplete adding models to enterprise owner work Nov 8, 2024
@danbrauer danbrauer force-pushed the gh_identity_enterprise_owners_models branch 2 times, most recently from 9ae8074 to 55f6f36 Compare November 8, 2024 14:38
@danbrauer danbrauer force-pushed the gh_identity_enterprise_owners_models branch 4 times, most recently from 6979bd5 to 24740d9 Compare November 8, 2024 20:39
@danbrauer danbrauer force-pushed the gh_identity_enterprise_owners_models branch from 24740d9 to eeb4729 Compare November 8, 2024 20:41
@danbrauer
Copy link
Author

Draft comment for when I share this PR:

This PR is in the middle of being refactored. In the first commit, it was complete, but done without schemas because it was focused on the User node, and no schema existed for the User node. alex asked that a User schema be created and used. This PR begins that work.

In the process of doing this, I hit a blocker that, if left as is, will I think wipe out GitHubOrganization nodes from the graph. Which would be bad. Because of this, I also couldn't fully test all the other cases. So, this is very much WIP, and THIS SHOULD NOT BE MERGED. I am sharing to get feedback on the blocker.

Details on the main blocking issue:

If you looked at the current loading Cypher, either in master or in this PR's first commit, you can see the first thing done is the creation of a GitHubOrganization node, if one does not already exist:

MERGE (org:GitHubOrganization{id: $OrgUrl})
ON CREATE SET org.firstseen = timestamp()
SET org.username = $OrgLogin,
org.lastupdated = $UpdateTag
WITH org

In contrast, if you look at the Cypher auto-generated from the new schema, there is no similar creation of GitHubOrganization. In case it helps, here is that Cypher code for one of the two loads in this PR:

        UNWIND $DictList AS item
            MERGE (i:GitHubUser{id: item.url})
            ON CREATE SET i.firstseen = timestamp()
            SET
                i.lastupdated = $lastupdated,
                i.fullname = item.name,
                i.username = item.login,
                i.is_site_admin = item.isSiteAdmin,
                i.is_enterprise_owner = item.isEnterpriseOwner,
                i.email = item.email,
                i.company = item.company,
                i.has_2fa_enabled = item.hasTwoFactorEnabled,
                i.role = item.role
            
        WITH i, item
        CALL {
            
        WITH i, item
        OPTIONAL MATCH (n0:GitHubOrganization)
        WHERE
            n0.id = item.MEMBER_OF
        WITH i, item, n0 WHERE n0 IS NOT NULL
        MERGE (i)-[r0:MEMBER_OF]->(n0)
        ON CREATE SET r0.firstseen = timestamp()
        SET
            r0.lastupdated = $lastupdated
        
        }

So, as far as I can see, the org will not be created. I have thoughts, but, before continuing, I wanted to pause and ask if this makes sense and if I am just misunderstanding something or doing something incorrectly. Thank you!

@danbrauer danbrauer merged commit a8009f9 into gh_identify_enterprise_owners Nov 8, 2024
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.

1 participant