Support enterprise hosted github instances #143

collinmurd · 2025-01-07T19:35:42Z

This adds support for GitHub Enterprise Server under a different hostname.

GITHUB_SERVER_URL is a default environment variable that is set on the runner to the url of whatever github instance it's associated with.

I only have my enterprise's environment to test with, this should be tested against github.com as well.

monsagri

Thank you for raising this @collinmurd!

I've left a couple of comments, one of them depending on us releasing an updated version this week

config/config.go

Co-authored-by: Fabian <[email protected]>

collinmurd · 2025-01-13T13:13:35Z

Thanks @monsagri! I'll keep an eye on your PR to upgrade go-github. After that's merged, I'll pull those changes here and test in my enterprise environment.

monsagri · 2025-01-15T15:04:26Z

Thanks @monsagri! I'll keep an eye on your PR to upgrade go-github. After that's merged, I'll pull those changes here and test in my enterprise environment.

Hey @collinmurd - We've just published the new version, would you be able to merge and give it a spin now?

collinmurd · 2025-01-15T19:53:04Z

Hey @monsagri. I'm running into an issue that may or may not be separate from this change.

The action is unable to create flag links in the app (with create-flag-links: true), and it fails with a seg fault:

Preprocessing diffs...
Scanning diff for references...
Summarizing results::debug::Setting outputs...
Processing comment...
Adding flag links...
  panic: runtime error: invalid memory address or nil pointer dereference
  [signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x8c2b4c]
  
  goroutine 1 [running]:
  github.com/launchdarkly/find-code-references-in-pull-request/internal/ldclient.makeFlagLinkRep(0xc0001ec910, {0xc00045ade0, 0x12}, {0xc0009546d0, 0xa})
  	/app/internal/ldclient/flag_links.go:107 +0x1cc
  github.com/launchdarkly/find-code-references-in-pull-request/internal/ldclient.CreateFlagLinks(0xc0001b6360, {0xc00012a060?, 0xc00012a090?, 0xc00012a0c0?}, 0xc0001ec910)
  	/app/internal/ldclient/flag_links.go:[33](https://github.nwie.net/Nationwide/nwag-boot-utils/actions/runs/144478/job/2602746#step:5:37) +0x171
  main.main()
  	/app/main.go:105 +0xc15

I've confirmed this happens both before merging in the go-github update and after. Also confirmed my LD token has appropriate permissions. I missed this in my initial testing because it has an odd behavior.

The first time this runs on a PR, it successfully creates the LaunchDarkly flag references PR comment, but exits with the error above and fails to create flag links. If I re-run the workflow, the action will succeed without error, but still fail to create flag links. If re-run again, but delete the PR comment, it once again exits with error.

I'm doing some debugging, but this is my first time in Go, so it might be a slow process. If you have any ideas let me know!

At this point I'm not sure whether this issue is due to the integration with GitHub Enterprise, or if it's something to do with our self-hosted Actions runners, or even something to do with the change I made. So if you guys would prefer, I could close this PR for now and create an issue instead.

monsagri

Not requesting changes to block, just realised I approved prematurely in my past review

monsagri · 2025-01-16T09:36:03Z

Hey @monsagri. I'm running into an issue that may or may not be separate from this change.

The action is unable to create flag links in the app (with create-flag-links: true), and it fails with a seg fault:
Preprocessing diffs...
Scanning diff for references...
Summarizing results::debug::Setting outputs...
Processing comment...
Adding flag links...
  panic: runtime error: invalid memory address or nil pointer dereference
  [signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x8c2b4c]
  
  goroutine 1 [running]:
  github.com/launchdarkly/find-code-references-in-pull-request/internal/ldclient.makeFlagLinkRep(0xc0001ec910, {0xc00045ade0, 0x12}, {0xc0009546d0, 0xa})
  	/app/internal/ldclient/flag_links.go:107 +0x1cc
  github.com/launchdarkly/find-code-references-in-pull-request/internal/ldclient.CreateFlagLinks(0xc0001b6360, {0xc00012a060?, 0xc00012a090?, 0xc00012a0c0?}, 0xc0001ec910)
  	/app/internal/ldclient/flag_links.go:[33](https://github.nwie.net/Nationwide/nwag-boot-utils/actions/runs/144478/job/2602746#step:5:37) +0x171
  main.main()
  	/app/main.go:105 +0xc15
I've confirmed this happens both before merging in the go-github update and after. Also confirmed my LD token has appropriate permissions. I missed this in my initial testing because it has an odd behavior.

The first time this runs on a PR, it successfully creates the LaunchDarkly flag references PR comment, but exits with the error above and fails to create flag links. If I re-run the workflow, the action will succeed without error, but still fail to create flag links. If re-run again, but delete the PR comment, it once again exits with error.

I'm doing some debugging, but this is my first time in Go, so it might be a slow process. If you have any ideas let me know!

At this point I'm not sure whether this issue is due to the integration with GitHub Enterprise, or if it's something to do with our self-hosted Actions runners, or even something to do with the change I made. So if you guys would prefer, I could close this PR for now and create an issue instead.

Thanks for the detailed information on this @collinmurd!

I'll take a look myself, hopefully I'll be able to reproduce and figure out what's going on there

monsagri · 2025-01-16T09:36:55Z

Hey @monsagri. I'm running into an issue that may or may not be separate from this change.

The action is unable to create flag links in the app (with create-flag-links: true), and it fails with a seg fault:
Preprocessing diffs...
Scanning diff for references...
Summarizing results::debug::Setting outputs...
Processing comment...
Adding flag links...
  panic: runtime error: invalid memory address or nil pointer dereference
  [signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x8c2b4c]
  
  goroutine 1 [running]:
  github.com/launchdarkly/find-code-references-in-pull-request/internal/ldclient.makeFlagLinkRep(0xc0001ec910, {0xc00045ade0, 0x12}, {0xc0009546d0, 0xa})
  	/app/internal/ldclient/flag_links.go:107 +0x1cc
  github.com/launchdarkly/find-code-references-in-pull-request/internal/ldclient.CreateFlagLinks(0xc0001b6360, {0xc00012a060?, 0xc00012a090?, 0xc00012a0c0?}, 0xc0001ec910)
  	/app/internal/ldclient/flag_links.go:[33](https://github.nwie.net/Nationwide/nwag-boot-utils/actions/runs/144478/job/2602746#step:5:37) +0x171
  main.main()
  	/app/main.go:105 +0xc15
I've confirmed this happens both before merging in the go-github update and after. Also confirmed my LD token has appropriate permissions. I missed this in my initial testing because it has an odd behavior.

The first time this runs on a PR, it successfully creates the LaunchDarkly flag references PR comment, but exits with the error above and fails to create flag links. If I re-run the workflow, the action will succeed without error, but still fail to create flag links. If re-run again, but delete the PR comment, it once again exits with error.

I'm doing some debugging, but this is my first time in Go, so it might be a slow process. If you have any ideas let me know!

At this point I'm not sure whether this issue is due to the integration with GitHub Enterprise, or if it's something to do with our self-hosted Actions runners, or even something to do with the change I made. So if you guys would prefer, I could close this PR for now and create an issue instead.

If it's easier for you, definitely feel free to close and let us take over. We really appreciate the effort you've put in already, and I'm planning on getting this fixed either way, so I'm happy to simply take the work you've done so far and build from there!

collinmurd · 2025-01-16T14:24:27Z

@monsagri I discovered that an empty PR body will show up as null in the event. That was causing the nil dereference.

I added a change to this PR to handle that case, but let me know if there's a more idiomatic way of doing it.

monsagri · 2025-01-16T14:39:01Z

@monsagri I discovered that an empty PR body will show up as null in the event. That was causing the nil dereference.

I added a change to this PR to handle that case, but let me know if there's a more idiomatic way of doing it.

Hey @collinmurd!

I actually ended up branching off this PR branch earlier today and merging #147 to pull in your changes with a few fixes for the potentially empty PR body and other fields that might've led to null pointers.
I also added in a fallback value for the description field, as it is required by the flag links API.

I've just cut a new release 2.1.0, which includes these changes, hopefully that should fix things for you.
Unfortunately I've not been able to get a GH Enterprise hosted instance to test things with just yet, would you mind giving it a try?

collinmurd · 2025-01-16T17:19:26Z

Aha! Works like a charm. Thank you for the partnership @monsagri!

monsagri · 2025-01-16T18:00:16Z

Thank you for helping out and raising the PR @collinmurd !
I made sure to branch off it since I wanted your work to show up under your name :D

All the best, I'm glad it works now!

create github client with url based on the environment

46bb04a

collinmurd requested a review from a team as a code owner January 7, 2025 19:35

collinmurd mentioned this pull request Jan 7, 2025

"repo-token lacks required permissions" #102

Closed

monsagri mentioned this pull request Jan 9, 2025

bump go-github to current v68 #145

Merged

monsagri approved these changes Jan 13, 2025

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

config/config.go Outdated Show resolved Hide resolved

collinmurd and others added 2 commits January 13, 2025 08:00

Update config/config.go

0bf5d91

Co-authored-by: Fabian <[email protected]>

only use enterprise client if necessary

172cb6c

monsagri self-requested a review January 15, 2025 15:04

Merge branch 'main' into main

9d7a531

monsagri requested changes Jan 16, 2025

View reviewed changes

handle a nil (empty) PR body

e3dbae6

collinmurd closed this Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support enterprise hosted github instances #143

Support enterprise hosted github instances #143

collinmurd commented Jan 7, 2025 •

edited

Loading

monsagri left a comment

collinmurd commented Jan 13, 2025 •

edited

Loading

monsagri commented Jan 15, 2025

collinmurd commented Jan 15, 2025 •

edited

Loading

monsagri left a comment

monsagri commented Jan 16, 2025

monsagri commented Jan 16, 2025

collinmurd commented Jan 16, 2025

monsagri commented Jan 16, 2025

collinmurd commented Jan 16, 2025

monsagri commented Jan 16, 2025

Support enterprise hosted github instances #143

Support enterprise hosted github instances #143

Conversation

collinmurd commented Jan 7, 2025 • edited Loading

monsagri left a comment

Choose a reason for hiding this comment

collinmurd commented Jan 13, 2025 • edited Loading

monsagri commented Jan 15, 2025

collinmurd commented Jan 15, 2025 • edited Loading

monsagri left a comment

Choose a reason for hiding this comment

monsagri commented Jan 16, 2025

monsagri commented Jan 16, 2025

collinmurd commented Jan 16, 2025

monsagri commented Jan 16, 2025

collinmurd commented Jan 16, 2025

monsagri commented Jan 16, 2025

collinmurd commented Jan 7, 2025 •

edited

Loading

collinmurd commented Jan 13, 2025 •

edited

Loading

collinmurd commented Jan 15, 2025 •

edited

Loading