Skip to content

Avoid double counting outside collaborators, preventing deletions #810

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

Merged

Conversation

klutchell
Copy link
Contributor

@klutchell klutchell commented Apr 14, 2025

The 'direct' affiliation of collaborators includes the 'outside' collaborators,
and in testing when the resulting source contained duplicates the test
function was not deleting collaborators.

@Copilot Copilot AI review requested due to automatic review settings April 14, 2025 17:29
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

lib/plugins/diffable.js:93

  • Ensure that JSON.stringify is used consistently for logging all objects to maintain clear and accurate debug output.
this.log.debug(`Results of comparing ${this.constructor.name} diffable target ${JSON.stringify(existingRecords)} with source ${JSON.stringify(filteredEntries)} is ${JSON.stringify(results)}`)

lib/plugins/diffable.js:95

  • Verify that using 'this.repo.repo' is correct regarding the structure of the repo object, ensuring consistency with related usages elsewhere in the code.
this.log.debug(`There are no changes for ${this.constructor.name} for repo ${this.repo.repo}. Skipping changes`)

lib/plugins/collaborators.js:23

  • Confirm that removing the API call for 'outside' affiliation fully prevents double counting while still capturing all necessary collaborator data.
return Promise.all([this.github.repos.listCollaborators({ repo: this.repo.repo, owner: this.repo.owner, affiliation: 'direct' }),

@klutchell
Copy link
Contributor Author

@decyjphr this one took me a while to track down, but the answer turned out to be in the REST docs if read very carefully

Copy link
Collaborator

@decyjphr decyjphr left a comment

Choose a reason for hiding this comment

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

Good catch!

@klutchell klutchell force-pushed the balena/dedupe-collaborators branch from 6550558 to ce15e59 Compare April 15, 2025 18:29
@klutchell
Copy link
Contributor Author

Oops, caught the failing test too. Thanks @decyjphr !

@decyjphr
Copy link
Collaborator

Looks good! Once your code has the latest from the base I can merge.

The 'direct' affiliation of collaborators includes the 'outside' collaborators,
and in testing when the resulting source contained duplicates the test
function was not deleting collaborators.

Signed-off-by: Kyle Harding <[email protected]>
@klutchell klutchell force-pushed the balena/dedupe-collaborators branch from ce15e59 to fd4af7e Compare April 15, 2025 18:43
@decyjphr decyjphr merged commit b0805f0 into github:main-enterprise Apr 15, 2025
2 checks passed
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.

2 participants