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

Specify node version #67

Merged
merged 12 commits into from
Feb 20, 2025

Conversation

ChrisStrykesAgain
Copy link
Collaborator

@ChrisStrykesAgain ChrisStrykesAgain commented Feb 19, 2025

This is work required for #64, but doesn't complete that issue fully. That will be done in a subsequent PR in an attempt to be a little more surgical with my changes and avoid a kitchen sink PR.

@ChrisStrykesAgain ChrisStrykesAgain marked this pull request as ready for review February 19, 2025 14:50
@ChrisStrykesAgain
Copy link
Collaborator Author

@rajsite @cameronwaterman @atmgrifter00 These changes lay the foundation for the rest of my PRs. All I've done here is set a specific node version for the build system (which matches the previously assumed default). In a subsequent PRs I'll update it to use node 20, but to avoid a kitchen sink PR I'm trying to be a bit more surgical.

@rajsite
Copy link
Member

rajsite commented Feb 19, 2025

@atmgrifter00 @cameronwaterman is there a reason it wasn't merged after finishing review as code owners? What's the expected workflow?

@ChrisStrykesAgain you had mentioned a concern about merge timings does that apply here? Are we waiting for merge?

@cameronwaterman
Copy link
Collaborator

@rajsite - Are you suggesting we should auto-merge after owners review? I expected we followed a similar pattern as we do with AzDO where the PR creator merges the PR when they're ready.

@rajsite
Copy link
Member

rajsite commented Feb 19, 2025

@rajsite - Are you suggesting we should auto-merge after owners review? I expected we followed a similar pattern as we do with AzDO where the PR creator merges the PR when they're ready.

On GitHub only collaborators are allowed to merge source code, i.e. PRs can be opened by anyone as these are public projects but can only be merged into the codebase by those with write access. There are patterns to say give all NI employees (those in the ni github org) write access to merge approved PRs but that is not how this repo is configured.

@cameronwaterman
Copy link
Collaborator

@rajsite - Are you suggesting we should auto-merge after owners review? I expected we followed a similar pattern as we do with AzDO where the PR creator merges the PR when they're ready.

On GitHub only collaborators are allowed to merge source code, i.e. PRs can be opened by anyone as these are public projects but can only be merged into the codebase by those with write access. There are patterns to say give all NI employees (those in the ni github org) write access to merge approved PRs but that is not how this repo is configured.

I see - I thought that functionality was opt-out instead of opt-in. Shows you how much I use GitHub 😆

I just added Chris as a "write" Collaborator so he can at least merge his own PRs when he's ready.

@ChrisStrykesAgain ChrisStrykesAgain merged commit cb90411 into ni:master Feb 20, 2025
1 check passed
@ChrisStrykesAgain ChrisStrykesAgain deleted the specify-node-version branch February 20, 2025 15:03
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.

4 participants