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

Support enterprise hosted github instances #143

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

collinmurd
Copy link

@collinmurd collinmurd commented Jan 7, 2025

see #102

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.

Copy link
Contributor

@monsagri monsagri left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
collinmurd and others added 2 commits January 13, 2025 08:00
@collinmurd
Copy link
Author

collinmurd commented Jan 13, 2025

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
Copy link
Contributor

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?

@monsagri monsagri self-requested a review January 15, 2025 15:04
@collinmurd
Copy link
Author

collinmurd commented Jan 15, 2025

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.

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