Skip to content

feat!: Add support for creating GitHub App with organizations - #3222

Merged
gmlewis merged 8 commits into
google:masterfrom
wenked:master
Jul 29, 2024
Merged

feat!: Add support for creating GitHub App with organizations#3222
gmlewis merged 8 commits into
google:masterfrom
wenked:master

Conversation

@wenked

@wenkedwenked commented Jul 29, 2024

Copy link
Copy Markdown
Contributor

Fixes: #3210.

BREAKING CHANGE: The CreateApp function now requires two arguments: AppManifest and orgName, to support creating apps with organizations.

The issue occurred because the create app URL path was only set up for creating personal GitHub apps.

My solution was to add an additional argument to the function to provide the necessary information for creating organizational GitHub Apps. The second argument, AppConfig, now includes configuration structs with an OwnerType that can be either personal or organizational, and you can provide the organization name by passing OrgName in the AppConfig struct.

For more details, see the issue: #3210.

@google-cla

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @wenked !
Please address the few minor comments.

@willnorris - since this involves scrape, do you have any comments or concerns regarding the breaking change in this PR?

Comment threadscrape/scrape.go Outdated
Comment threadscrape/apps.go Outdated
Comment threadscrape/apps.go Outdated
Comment threadscrape/apps.go Outdated
Comment threadscrape/apps.go Outdated
@gmlewis

Copy link
Copy Markdown
Collaborator

We'll see what @willnorris says, but we could alternatively have a second ...ForOrgs method if the breaking API change is unwanted.

@gmlewisgmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Jul 29, 2024
@codecov

codecovBot commented Jul 29, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.94%. Comparing base (2b8c7fa) to head (d6cf6fa).
Report is 89 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #3222 +/- ##
==========================================
- Coverage 97.72% 92.94% -4.78% 
==========================================
Files 153 171 +18 Lines 13390 11633 -1757 ==========================================
- Hits 13085 10812 -2273 - Misses 215 727 +512 - Partials 90 94 +4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@willnorriswillnorris left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't have any sense on who is using this package (not many people, looking at pkg.go.dev), so I don't have strong feelings either way on the change. looks fine to me.

Comment threadscrape/apps.go Outdated
wenkedand others added 3 commits July 29, 2024 18:58
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Jul 29, 2024

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @wenked !
LGTM.
Merging.

@gmlewis
gmlewis merged commit f8929b8 into google:masterJul 29, 2024
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
…#3222)
Fixes: google#3210.
BREAKING CHANGE: The CreateApp function now requires two arguments: `AppManifest` and `orgName`, to support creating apps with organizations.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking API ChangePR will require a bump to the major version num in next release. Look here to see the change(s).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for GitHub App Manifest flow on Organization, not User

3 participants

@wenked@gmlewis@willnorris