Skip to content

Conversation

@gagik
Copy link
Contributor

@gagik gagik commented Feb 19, 2025

A lot of the helper do dependency injection by using some of its arguments to optionally pass other (stubbed) helpers. This is getting out of hand and now we have things like:

export async function publishMongosh(
  config: Config,
  mongoshGithubRepo: GithubRepo,
  mongodbHomebrewForkGithubRepo: GithubRepo,
  homebrewCoreGithubRepo: GithubRepo,
  barque: Barque,
  createAndPublishDownloadCenterConfig: typeof createAndPublishDownloadCenterConfigFn,
  publishToNpm: typeof publishToNpmType,
  pushTags: typeof pushTagsType,
  writeBuildInfo: typeof writeBuildInfoType,
  publishToHomebrew: typeof publishToHomebrewType,
  shouldDoPublicRelease: typeof shouldDoPublicReleaseFn = shouldDoPublicReleaseFn,
  getEvergreenArtifactUrl: typeof getArtifactUrlFn = getArtifactUrlFn,
  bumpMongoshReleasePackages: typeof bumpMongoshReleasePackagesFn = bumpMongoshReleasePackagesFn,
  bumpAuxiliaryPackages: typeof bumpAuxiliaryPackagesFn = bumpAuxiliaryPackagesFn,
  spawnSync: typeof spawnSyncFn = spawnSyncFn 
...

To minimize this, this PR restructures these functions to be inside classes based on responsibility.

There are no real functionality changes beyond combining some helpers into a single class.
Some noteworthy changes:

  1. push-tags => combined with publish in PackagePublisher
  2. markBumpedFilesAsAssumeUnchanged was completely removed as it was unused.

For the sake of keeping this not larger than it already is, some helper functions were left the same and only more core ones were changed.

I am not changing the file names here so diffs can be more readable but it'd be worth doing that in a follow-up PR.

@gagik gagik changed the title refactor: restructure build helpers to use classes MONGOSH-2007 refactor(build): restructure build helpers to use classes MONGOSH-2007 Feb 20, 2025
fsWriteFile.resolves();

testBumper = new PackageBumper();
sinon.stub(testBumper, 'spawnSync').resolves();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we turn these 'stored as properties with the expectation to mock them later' methods into regular properties that get passed through the constructor?

The previous dependency injection code was primarily annoying imo because functions would take more and more arguments, but the general principle of explicitly passing in a dependency is still a good one

@gagik gagik force-pushed the gagik/build-refactor branch from ee49654 to f7ae228 Compare February 25, 2025 12:54
@gagik gagik merged commit 78ad910 into main Feb 26, 2025
115 of 119 checks passed
@gagik gagik deleted the gagik/build-refactor branch February 26, 2025 09:04
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.

3 participants