fix(ng-dev): merge tool accidentally performing unauthenticated Githu…

…b requests (#228)

The merge tool accidentally performs unauthenticated Github API
requests. This started happening when we refactored some things
in the merge tool.

We should not rely on default values for parameters as explicit
parameters encourage thinking about passing the potentially needed
/already-existing values (like for the authenctiated git client).

This commit changes this in all places of the merge tool and
release tool in order to ensure we always use the correct
instance of the Git client (as initially configured).

Note: We could theoretically have a method that returns a git
client singleton, priortizing the authenticated one. Though I
personally still believe that being explicit is better and encouraging
thinking of function consumers.

ng-dev/pr/check-target-branches/check-target-branches.ts

@@ -7,16 +7,10 @@
  */
 
 import {assertValidGithubConfig, getConfig, GithubConfig} from '../../utils/config';
-import {error, info, red} from '../../utils/console';
+import {info} from '../../utils/console';
 import {GitClient} from '../../utils/git/git-client';
 import {assertValidMergeConfig, MergeConfig} from '../merge/config';
-import {
-  getBranchesFromTargetLabel,
-  getMatchingTargetLabelForPullRequest,
-  getTargetBranchesForPullRequest,
-  InvalidTargetLabelError,
-  TargetLabel,
-} from '../merge/target-label';
+import {getTargetBranchesForPullRequest} from '../merge/target-label';
 
 async function getTargetBranchesForPr(
   prNumber: number,
@@ -40,7 +34,7 @@ async function getTargetBranchesForPr(
 
   // Note: We do not pass a list of commits here because we did not fetch this information
   // and the commits are only used for validation (which we can skip here).
-  return getTargetBranchesForPullRequest(config, labels, githubTargetBranch, []);
+  return getTargetBranchesForPullRequest(git.github, config, labels, githubTargetBranch, []);
 }
 
 export async function printTargetBranchesForPr(prNumber: number) {

ng-dev/pr/merge/defaults/integration.spec.ts

@@ -18,7 +18,6 @@ import {
   getMatchingTargetLabelForPullRequest,
   TargetLabel,
 } from '../target-label';
-import * as labelDefaults from './labels';
 
 import {fakeGithubPaginationResponse} from '../../../utils/testing/github-interception';
 import {getTargetLabelsForActiveReleaseTrains} from './labels';

ng-dev/pr/merge/defaults/labels.ts

@@ -23,6 +23,7 @@ import {
 } from '../target-label';
 
 import {assertActiveLtsBranch} from './lts-branch';
+import {GithubClient} from '../../../utils/git/github';
 
 /**
  * Gets a list of target labels which should be considered by the merge
@@ -32,14 +33,13 @@ import {assertActiveLtsBranch} from './lts-branch';
  * specifies versioning, branching and releasing for the Angular organization:
  * https://docs.google.com/document/d/197kVillDwx-RZtSVOBtPb4BBIAw0E9RT3q3v6DZkykU
  *
- * @param api Instance of an authenticated Github client.
- *   for the release train branches.
+ * @param api Instance of a Github client. Used to query for the release train branches.
  * @param config Configuration for the Github remote and release packages. Used to fetch
  *   NPM version data when LTS version branches are validated.
  */
 export async function getTargetLabelsForActiveReleaseTrains(
-  api = GitClient.get().github,
-  config = getConfig() as Partial<{github: GithubConfig; release: ReleaseConfig}>,
+  api: GithubClient,
+  config: Partial<{github: GithubConfig; release: ReleaseConfig}>,
 ): Promise<TargetLabel[]> {
   assertValidReleaseConfig(config);
   assertValidGithubConfig(config);

ng-dev/pr/merge/pull-request.ts

@@ -67,6 +67,7 @@ export async function loadAndValidatePullRequest(
   const githubTargetBranch = prData.baseRefName;
 
   const targetBranches = await getTargetBranchesForPullRequest(
+    git.github,
     {github: git.config.github, merge: config},
     labels,
     githubTargetBranch,

ng-dev/pr/merge/target-label.ts

@@ -12,6 +12,7 @@ import {GithubConfig} from '../../utils/config';
 import {Commit} from '../../commit-message/parse';
 import {assertChangesAllowForTargetLabel} from './validations';
 import {PullRequestFailure} from './failures';
+import {GithubClient} from '../../utils/git/github';
 
 /**
  * Enum capturing available target label names in the Angular organization. A target
@@ -95,6 +96,7 @@ export async function getMatchingTargetLabelForPullRequest(
 
 /** Get the branches the pull request should be merged into. */
 export async function getTargetBranchesForPullRequest(
+  api: GithubClient,
   config: {merge: MergeConfig; github: GithubConfig},
   labelsOnPullRequest: string[],
   githubTargetBranch: string,
@@ -109,7 +111,7 @@ export async function getTargetBranchesForPullRequest(
   // can lazily compute branches for a target label and throw. e.g. if an invalid target
   // label is applied, we want to exit the script gracefully with an error message.
   try {
-    const targetLabels = await getTargetLabelsForActiveReleaseTrains();
+    const targetLabels = await getTargetLabelsForActiveReleaseTrains(api, config);
     const matchingLabel = await getMatchingTargetLabelForPullRequest(
       config.merge,
       labelsOnPullRequest,

ng-dev/release/notes/changelog.spec.ts

@@ -15,23 +15,25 @@ describe('Changelog', () => {
       /* useSandboxGitClient */ false,
     );
     spyOn(GitClient, 'get').and.returnValue(gitClient);
-    changelog = Changelog.getChangelogFilePaths();
+    changelog = Changelog.getChangelogFilePaths(gitClient);
   });
 
   it('throws an error if it cannot find the anchor containing the version for an entry', () => {
-    expect(() => Changelog.prependEntryToChangelogFile('does not have version <a> tag')).toThrow();
+    expect(() =>
+      Changelog.prependEntryToChangelogFile(gitClient, 'does not have version <a> tag'),
+    ).toThrow();
   });
 
   it('throws an error if it cannot determine the version for an entry', () => {
     expect(() =>
-      Changelog.prependEntryToChangelogFile(createChangelogEntry('NotSemVer')),
+      Changelog.prependEntryToChangelogFile(gitClient, createChangelogEntry('NotSemVer')),
     ).toThrow();
   });
 
   it('concatenates the changelog entries into the changelog file with the split marker between', () => {
-    Changelog.prependEntryToChangelogFile(createChangelogEntry('1.0.0'));
-    Changelog.prependEntryToChangelogFile(createChangelogEntry('2.0.0'));
-    Changelog.prependEntryToChangelogFile(createChangelogEntry('3.0.0'));
+    Changelog.prependEntryToChangelogFile(gitClient, createChangelogEntry('1.0.0'));
+    Changelog.prependEntryToChangelogFile(gitClient, createChangelogEntry('2.0.0'));
+    Changelog.prependEntryToChangelogFile(gitClient, createChangelogEntry('3.0.0'));
 
     expect(readFileAsString(changelog.filePath)).toBe(
       dedent`
@@ -47,7 +49,7 @@ describe('Changelog', () => {
     `.trim(),
     );
 
-    Changelog.moveEntriesPriorToVersionToArchive(new SemVer('3.0.0'));
+    Changelog.moveEntriesPriorToVersionToArchive(gitClient, new SemVer('3.0.0'));
 
     expect(readFileAsString(changelog.archiveFilePath)).toBe(
       dedent`
@@ -66,19 +68,19 @@ describe('Changelog', () => {
     it('creates a new changelog file if one does not exist.', () => {
       expect(existsSync(changelog.filePath)).toBe(false);
 
-      Changelog.prependEntryToChangelogFile(createChangelogEntry('0.0.0'));
+      Changelog.prependEntryToChangelogFile(gitClient, createChangelogEntry('0.0.0'));
       expect(existsSync(changelog.filePath)).toBe(true);
     });
 
     it('should not include a split marker when only one changelog entry is in the changelog.', () => {
-      Changelog.prependEntryToChangelogFile(createChangelogEntry('0.0.0'));
+      Changelog.prependEntryToChangelogFile(gitClient, createChangelogEntry('0.0.0'));
 
       expect(readFileAsString(changelog.filePath)).not.toContain(splitMarker);
     });
 
     it('separates multiple changelog entries using a standard split marker', () => {
       for (let i = 0; i < 2; i++) {
-        Changelog.prependEntryToChangelogFile(createChangelogEntry(`0.0.${i}`));
+        Changelog.prependEntryToChangelogFile(gitClient, createChangelogEntry(`0.0.${i}`));
       }
 
       expect(readFileAsString(changelog.filePath)).toContain(splitMarker);
@@ -87,31 +89,31 @@ describe('Changelog', () => {
 
   describe('adds entries to the changelog archive', () => {
     it('only updates or creates the changelog archive if necessary', () => {
-      Changelog.prependEntryToChangelogFile(createChangelogEntry('1.0.0'));
+      Changelog.prependEntryToChangelogFile(gitClient, createChangelogEntry('1.0.0'));
       expect(existsSync(changelog.archiveFilePath)).toBe(false);
 
-      Changelog.moveEntriesPriorToVersionToArchive(new SemVer('1.0.0'));
+      Changelog.moveEntriesPriorToVersionToArchive(gitClient, new SemVer('1.0.0'));
       expect(existsSync(changelog.archiveFilePath)).toBe(false);
 
-      Changelog.moveEntriesPriorToVersionToArchive(new SemVer('2.0.0'));
+      Changelog.moveEntriesPriorToVersionToArchive(gitClient, new SemVer('2.0.0'));
       expect(existsSync(changelog.archiveFilePath)).toBe(true);
     });
 
     it('from the primary changelog older than a provided version', () => {
       Changelog.prependEntryToChangelogFile(
-        createChangelogEntry('1.0.0', 'This is version 1'),
         gitClient,
+        createChangelogEntry('1.0.0', 'This is version 1'),
       );
       Changelog.prependEntryToChangelogFile(
-        createChangelogEntry('2.0.0', 'This is version 2'),
         gitClient,
+        createChangelogEntry('2.0.0', 'This is version 2'),
       );
       Changelog.prependEntryToChangelogFile(
-        createChangelogEntry('3.0.0', 'This is version 3'),
         gitClient,
+        createChangelogEntry('3.0.0', 'This is version 3'),
       );
 
-      Changelog.moveEntriesPriorToVersionToArchive(new SemVer('3.0.0'));
+      Changelog.moveEntriesPriorToVersionToArchive(gitClient, new SemVer('3.0.0'));
       expect(readFileAsString(changelog.archiveFilePath)).toContain('version 1');
       expect(readFileAsString(changelog.archiveFilePath)).toContain('version 2');
       expect(readFileAsString(changelog.archiveFilePath)).not.toContain('version 3');

ng-dev/release/notes/changelog.ts

@@ -38,7 +38,7 @@ interface ChangelogEntry {
 
 export class Changelog {
   /** Prepend a changelog entry to the current changelog file. */
-  static prependEntryToChangelogFile(entry: string, git = GitClient.get()) {
+  static prependEntryToChangelogFile(git: GitClient, entry: string) {
     const changelog = new this(git);
     changelog.prependEntryToChangelogFile(entry);
   }
@@ -52,14 +52,14 @@ export class Changelog {
    * other changelog entries.  This allows for example, moving all changelog entries out of the
    * main changelog when a version moves out of support.
    */
-  static moveEntriesPriorToVersionToArchive(version: semver.SemVer, git = GitClient.get()) {
+  static moveEntriesPriorToVersionToArchive(git: GitClient, version: semver.SemVer) {
     const changelog = new this(git);
     changelog.moveEntriesPriorToVersionToArchive(version);
   }
 
   // TODO(josephperrott): Remove this after it is unused.
   /** Retrieve the file paths for the changelog files. */
-  static getChangelogFilePaths(git = GitClient.get()) {
+  static getChangelogFilePaths(git: GitClient) {
     return new this(git);
   }
 

ng-dev/release/notes/cli.ts

@@ -11,6 +11,7 @@ import {Arguments, Argv, CommandModule} from 'yargs';
 import {info} from '../../utils/console';
 
 import {ReleaseNotes} from './release-notes';
+import {GitClient} from '../../utils/git/git-client';
 
 /** Command line options for building a release. */
 export interface Options {
@@ -54,8 +55,10 @@ function builder(argv: Argv): Argv<Options> {
 
 /** Yargs command handler for generating release notes. */
 async function handler({releaseVersion, from, to, prependToChangelog, type}: Arguments<Options>) {
+  /** Git client to use for generating the release notes. */
+  const git = GitClient.get();
   /** The ReleaseNotes instance to generate release notes. */
-  const releaseNotes = await ReleaseNotes.forRange(releaseVersion, from, to);
+  const releaseNotes = await ReleaseNotes.forRange(git, releaseVersion, from, to);
 
   if (prependToChangelog) {
     await releaseNotes.prependEntryToChangelogFile();

ng-dev/release/notes/release-notes.ts

@@ -27,8 +27,7 @@ export const changelogPath = 'CHANGELOG.md';
 
 /** Release note generation. */
 export class ReleaseNotes {
-  static async forRange(version: semver.SemVer, baseRef: string, headRef: string) {
-    const git = GitClient.get();
+  static async forRange(git: GitClient, version: semver.SemVer, baseRef: string, headRef: string) {
     const commits = getCommitsForRangeWithDeduping(git, baseRef, headRef);
     return new ReleaseNotes(version, commits, git);
   }
@@ -67,7 +66,7 @@ export class ReleaseNotes {
    * provided by the GitClient.
    */
   async prependEntryToChangelogFile() {
-    Changelog.prependEntryToChangelogFile(await this.getChangelogEntry(), this.git);
+    Changelog.prependEntryToChangelogFile(this.git, await this.getChangelogEntry());
 
     // TODO(josephperrott): Remove file formatting calls.
     //   Upon reaching a standardized formatting for markdown files, rather than calling a formatter

ng-dev/release/publish/actions.ts

@@ -417,7 +417,12 @@ export abstract class ReleaseAction {
     ]);
 
     // Build release notes for commits from `<releaseNotesCompareTag>..HEAD`.
-    const releaseNotes = await ReleaseNotes.forRange(newVersion, releaseNotesCompareTag, 'HEAD');
+    const releaseNotes = await ReleaseNotes.forRange(
+      this.git,
+      newVersion,
+      releaseNotesCompareTag,
+      'HEAD',
+    );
 
     await this.updateProjectVersion(newVersion);
     await this.prependReleaseNotesToChangelog(releaseNotes);

ng-dev/release/publish/cli.ts

@@ -10,11 +10,11 @@ import {Argv, CommandModule} from 'yargs';
 
 import {assertValidGithubConfig, getConfig} from '../../utils/config';
 import {error, green, info, red, yellow} from '../../utils/console';
-import {GitClient} from '../../utils/git/git-client';
 import {addGithubTokenOption} from '../../utils/git/github-yargs';
 import {assertValidReleaseConfig} from '../config/index';
 
 import {CompletionState, ReleaseTool} from './index';
+import {AuthenticatedGitClient} from '../../utils/git/authenticated-git-client';
 
 /** Command line options for publishing a release. */
 export interface ReleasePublishOptions {
@@ -28,11 +28,11 @@ function builder(argv: Argv): Argv<ReleasePublishOptions> {
 
 /** Yargs command handler for staging a release. */
 async function handler() {
-  const git = GitClient.get();
+  const git = AuthenticatedGitClient.get();
   const config = getConfig();
   assertValidReleaseConfig(config);
   assertValidGithubConfig(config);
-  const task = new ReleaseTool(config.release, config.github, git.baseDir);
+  const task = new ReleaseTool(git, config.release, config.github, git.baseDir);
   const result = await task.run();
 
   switch (result) {

ng-dev/release/publish/index.ts

@@ -27,12 +27,11 @@ export enum CompletionState {
 }
 
 export class ReleaseTool {
-  /** The singleton instance of the authenticated git client. */
-  private _git = AuthenticatedGitClient.get();
   /** The previous git commit to return back to after the release tool runs. */
   private previousGitBranchOrRevision = this._git.getCurrentBranchOrRevision();
 
   constructor(
+    protected _git: AuthenticatedGitClient,
     protected _config: ReleaseConfig,
     protected _github: GithubConfig,
     protected _projectRoot: string,

ng-dev/release/publish/test/common.spec.ts

@@ -272,13 +272,18 @@ class TestAction extends ReleaseAction {
     distTag: NpmDistTag,
     releaseNotesCompareTag = 'HEAD',
   ) {
-    const releaseNotes = await ReleaseNotes.forRange(version, releaseNotesCompareTag, 'HEAD');
+    const releaseNotes = await ReleaseNotes.forRange(
+      this.git,
+      version,
+      releaseNotesCompareTag,
+      'HEAD',
+    );
     debugger;
     await this.buildAndPublish(releaseNotes, publishBranch, distTag);
   }
 
   async testCherryPickWithPullRequest(version: semver.SemVer, branch: string) {
-    const releaseNotes = await ReleaseNotes.forRange(version, '', '');
+    const releaseNotes = await ReleaseNotes.forRange(this.git, version, '', '');
     await this.cherryPickChangelogIntoNextBranch(releaseNotes, branch);
   }
 }