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

feat(release): support github enterprise server #26482

Merged
merged 9 commits into from
Sep 20, 2024

Conversation

JamesHenry
Copy link
Collaborator

Current Behavior

GitHub Enterprise Server is not supported by nx release.

Expected Behavior

GitHub Enterprise Server is supported by nx release.

Related Issue(s)

Fixes #

Copy link

vercel bot commented Jun 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview Sep 10, 2024 9:49am

Copy link

🐳 We have a release for that!

This PR has a release associated with it. You can try it out using this command:

npx [email protected] my-workspace

Or just copy this version and use it in your own command:

0.0.0-pr-26482-ebe2dba
Release details 📑
Published version 0.0.0-pr-26482-ebe2dba
Triggered by @JamesHenry
Branch release-github-enterprise-server
Commit ebe2dba
Workflow run 9500624198

To request a new release for this pull request, mention someone from the Nx team or the @nrwl/nx-pipelines-reviewers.

@Bare1337
Copy link

Bare1337 commented Jun 18, 2024

Hello again, and sorry about my delayed answer @JamesHenry . Thanks a lot for your reactivity, thoses PR releases are such an awesome capability by the way.

I was able to test your changes in our repository and the only "issue" left is about the regex which extracts the repo slug in getGithubRepoData.

const regexString = `${escapedHostname}[/:]([\\w-]+/[\\w-]+)`;

As github authorize '-', '_', and '.' as separators in the repository name (as well as any ASCII letters and digits), the regex is not able to capture the repo slug properly when a dot is used as a separator for example. (Which was the case in our context.)

After patching this, i was able to go through the whole release workflow and i'am happy to say it works perfectly with our configuration. We've tested both the default api url configuration as well as the custom one, and each time it was successful to create a github release on GHES.

Don't hesitate to let me know if you need further testing.

Thanks for the great work put in nx release again !

@xalechez
Copy link

Hey 👋 Do you know whether there're any plans to merge this PR in the near future?

@JamesHenry JamesHenry force-pushed the release-github-enterprise-server branch from ebe2dba to 7ad9136 Compare July 31, 2024 10:14
@JamesHenry
Copy link
Collaborator Author

Hi Folks, I'm updating the branch to include all the latest changes with Nx, and will cut another PR release once CI is green.

@Bare1337 thanks a lot for the feedback, please could you provide the updated regex as well as examples of valid inputs you are expecting it to support, I want to make sure we have tests for that?

@xalechez Have you been testing out the release? Do you have any feedback?

Copy link

🐳 We have a release for that!

This PR has a release associated with it. You can try it out using this command:

npx [email protected] my-workspace

Or just copy this version and use it in your own command:

0.0.0-pr-26482-1bdb7bd
Release details 📑
Published version 0.0.0-pr-26482-1bdb7bd
Triggered by @JamesHenry
Branch release-github-enterprise-server
Commit 1bdb7bd
Workflow run 10181344373

To request a new release for this pull request, mention someone from the Nx team or the @nrwl/nx-pipelines-reviewers.

@pcopley
Copy link

pcopley commented Jul 31, 2024

@JamesHenry Thanks so much for your work on this! How do I upgrade an existing workspace to this PR release? I've tried nx migrate 0.0.0-pr-26482-1bdb7bd but get this error:

❯ nx migrate 0.0.0-pr-26482-1bdb7bd
Fetching meta data about packages.
It may take a few minutes.
Fetching @nrwl/[email protected]
Unable to fetch migrations for @nrwl/[email protected]: Command failed: npm install @nrwl/[email protected]
npm error code ETARGET
npm error notarget No matching version found for @nrwl/[email protected].
npm error notarget In most cases you or one of your dependencies are requesting
npm error notarget a package version that doesn't exist.

@JamesHenry
Copy link
Collaborator Author

JamesHenry commented Jul 31, 2024

@pcopley We maybe need to look into special handling in migrate for PR releases.

There aren't any migrations involved here in this PR though, so I would just nx migrate latest, and then patch your nx and @nx/* package versions in your package.json and run install

@xalechez
Copy link

xalechez commented Aug 1, 2024

Have you been testing out the release? Do you have any feedback?

Hey @JamesHenry 👋 Yes, I've tested release in GHE, and it went smoothly (in our case with both previous and latest commits)

@ryan-bendel
Copy link

Watching this for when it gets merged. Tested the package above and works on our GHE instance 👍

@JamesHenry
Copy link
Collaborator Author

I'm still waiting to hear back from @Bare1337 regarding the regexp. Did you not run into this @xalechez and @ryan-bendel?

@ryan-bendel
Copy link

ryan-bendel commented Aug 12, 2024

@JamesHenry - I saw no issues on our GHE server, but not sure how they would have presented?

I opted to edit the release notes prior to posting, so will give it a try creating the release "as is"

@matinzd
Copy link

matinzd commented Aug 12, 2024

Actually I ran into the issue today. :)

Do you want me to test this PR?

 NX   Unable to create a GitHub release because the GitHub repo slug could not be determined.

@xalechez
Copy link

I'm still waiting to hear back from @Bare1337 regarding the regexp. Did you not run into this @xalechez and @ryan-bendel?

No, I guess our GHE URL satisfies the regex in question

@ryan-bendel
Copy link

I did actually run into an issue when trying to publish the release:

A GitHub API Error occurred when creating/updating the release

GitHub Error: {"message":"Not Found","documentation_url":"https://docs.github.com/[email protected]/rest/releases/releases#create-a-release"}

Request Data:
Repo: MyCompanyName/MyRepo
Token: null
Body: {"tag_name":"v0.1.3","name":"v0.1.3","body":"## 0.1.3 (2024-08-12)\n\nThis was a version bump only, there were no code changes.","prerelease":false}

(that's when i don't choose to open in browser to edit the generated release notes)

I've not updated any of the other NX packages though, i've only updated "nx": "0.0.0-pr-26482-1bdb7bd", - so not sure if that's the issue

@JamesHenry
Copy link
Collaborator Author

@ryan-bendel In general I would always say that you should align the nx and all the @nx packages, there is no reason not to, and not doing so opens you up to weird incompatibilities.

Interesting in your case here though regarding editing the changelog vs not. Is there a reason to hide the repo URL? That's the all important thing here so without it we can't dig in much further... Security through obscurity won't be doing much for a GHE instance I don't think, but if you feel more comfortable you can DM me the info: https://x.com/MrJamesHenry

@ryan-bendel
Copy link

@ryan-bendel In general I would always say that you should align the nx and all the @nx packages, there is no reason not to, and not doing so opens you up to weird incompatibilities.

Interesting in your case here though regarding editing the changelog vs not. Is there a reason to hide the repo URL? That's the all important thing here so without it we can't dig in much further... Security through obscurity won't be doing much for a GHE instance I don't think, but if you feel more comfortable you can DM me the info: https://x.com/MrJamesHenry

I'll bump all my version to match 👍

My config is:

      "createRelease": {
          "apiBaseUrl": "https://github.atcloud.io/api/v3/",
          "hostname": "github.atcloud.io",
          "provider": "github-enterprise-server"
        },

And the repo url it mentions is Repo: AutoTrader/nxtestrepo

@JamesHenry
Copy link
Collaborator Author

Thanks @ryan-bendel and just to check, when you said edit the release notes before publishing in the successful case, were you referring to the local code editor workflow, or actually using the GitHub UI for editing the release notes?

What are you using for auth? An env var? Or a gh CLI session?

@ryan-bendel
Copy link

Thanks @ryan-bendel and just to check, when you said edit the release notes before publishing in the successful case, were you referring to the local code editor workflow, or actually using the GitHub UI for editing the release notes?

What are you using for auth? An env var? Or a gh CLI session?

No prob @JamesHenry - Github UI was when it had "worked". That error is thrown prior to the prompt Do you want to finish creating the release manually in your browser?.

Auth is via gh cli. Generated a new personal access token and made sure gh auth login and gh auth status all looked good

@JamesHenry
Copy link
Collaborator Author

@ryan-bendel That sounds like the auth is not working for you as expected for nx release then.

Here you can see our logic for attempting to reuse the gh session for authenticating the release:

export async function resolveGithubToken(): Promise<string | null> {
// Try and resolve from the environment
const tokenFromEnv = process.env.GITHUB_TOKEN || process.env.GH_TOKEN;
if (tokenFromEnv) {
return tokenFromEnv;
}
// Try and resolve from gh CLI installation
const ghCLIPath = joinPathFragments(
process.env.XDG_CONFIG_HOME || joinPathFragments(homedir(), '.config'),
'gh',
'hosts.yml'
);
if (existsSync(ghCLIPath)) {
const yamlContents = await fsp.readFile(ghCLIPath, 'utf8');
const { load } = require('@zkochan/js-yaml');
const ghCLIConfig = load(yamlContents);
if (ghCLIConfig['github.com']) {
// Web based session (the token is already embedded in the config)
if (ghCLIConfig['github.com'].oauth_token) {
return ghCLIConfig['github.com'].oauth_token;
}
// SSH based session (we need to dynamically resolve a token using the CLI)
if (
ghCLIConfig['github.com'].user &&
ghCLIConfig['github.com'].git_protocol === 'ssh'
) {
return execSync(`gh auth token`, {
encoding: 'utf8',
stdio: 'pipe',
}).trim();
}
}
}
return null;
}

The Do you want to finish creating the release manually in your browser? only happens when there is an error, and the error you are getting strongly suggests auth.

Please can you step through that logic in your node_modules while running the release and see why it is not finding a token?

Many thanks

@JamesHenry
Copy link
Collaborator Author

JamesHenry commented Aug 13, 2024

Oh wait, ha - no sooner did I post that than I realized we are explicitly checking for github.com in there :D

@ryan-bendel Please can you confirm in your relevant gh hosts.yml file on your machine your token is stored under an entry that matches your github.atcloud.io value? (Naturally don't share the token value here)

@ryan-bendel
Copy link

ryan-bendel commented Aug 13, 2024

@JamesHenry - got it working now by setting GH_TOKEN in my .env file

Oh wait, ha - no sooner did I post that than I realized we are explicitly checking for github.com in there :D

@ryan-bendel Please can you confirm in your relevant gh hosts.yml file on your machine your token is stored under an entry that matches your github.atcloud.io value? (Naturally don't share the token value here)

I can't see the token itself in there (I think the token is stored in the credentials store?)

github.atcloud.io:
    git_protocol: https
    users:
        ryan-bendel:
    user: ryan-bendel

But doing a gh auth status I see:

  ✓ Logged in to github.atcloud.io account ryan-bendel (keyring)
  - Active account: true
  - Git operations protocol: https
  - Token: ghp_************************************
  - Token scopes: 'admin:org', 'admin:org_hook', 'admin:public_key', 'admin:repo_hook', 'project', 'repo', 'user', 'workflow'

I've tested adding that token to my .env file in the repo GH_TOKEN=<token> - and it works as expected :)

@JamesHenry
Copy link
Collaborator Author

Ok thanks for confirming, I think then we can potentially add some additional handling for non github.com cases in the auth logic - we know we won't find the token in this case, so we can get folks to always use the env var method.

@ryan-bendel
Copy link

Ok thanks for confirming, I think then we can potentially add some additional handling for non github.com cases in the auth logic - we know we won't find the token in this case, so we can get folks to always use the env var method.

Sounds like a plan @JamesHenry - nothing a bit of documentation can't sort out. And tbh, it's a bit of an easier dev experience getting teams/users to generate a personal access token and sticking it in their .env than making them install the gh cli anyway

Thanks for looking into it with me! 👍

@matinzd
Copy link

matinzd commented Aug 13, 2024

Is it also possible to add interactive/manual support for Github release creation?
I think it's possible to just create new Github release with query params (pre-populated fields) by opening the browser.
release-it uses this approach.

https://github.com/release-it/release-it?tab=readme-ov-file#github-releases

GitHub projects can have releases attached to Git tags, containing release notes and assets. There are two ways to add GitHub releases in your release-it flow:

Automated (requires a GITHUB_TOKEN)
Manual (using the GitHub web interface with pre-populated fields)

WDYT?

@JamesHenry
Copy link
Collaborator Author

@matinzd That is the exact thing we are discussing (Do you want to finish creating the release manually in your browser? as referenced above), it is launched automatically when the auth does not work, nx release has had this for ages

@matinzd
Copy link

matinzd commented Aug 13, 2024

Yeah I missed those replies 👍

@nalham
Copy link

nalham commented Sep 4, 2024

Is there a targeted release for this change?

@JamesHenry
Copy link
Collaborator Author

JamesHenry commented Sep 9, 2024

I have just brought the branch up to date with the latest.

I believe the remaining minor TODOs are:

Update: all addressed in 60b4f00

This PR is now just pending review from the team

@JamesHenry JamesHenry marked this pull request as ready for review September 10, 2024 10:20
@JamesHenry JamesHenry requested a review from a team as a code owner September 10, 2024 10:20
@JamesHenry JamesHenry requested a review from xiongemi September 10, 2024 10:20
@JamesHenry JamesHenry merged commit 21d1696 into master Sep 20, 2024
6 checks passed
@JamesHenry JamesHenry deleted the release-github-enterprise-server branch September 20, 2024 12:15
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants