Skip to content

Conversation

@Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Mar 19, 2025

Problem

Both prepareRepoData and zipUtil are ways to take your current workspace and package it up as a zip. /doc and /dev use prepareRepoData. /review and /test use zipUtil. Their implementations are pretty similar and we should be able to have a shared utility that is a consolidated version of the two.

Both implementations are highly coupled to their use case, but more so zipUtil. Additionally, zipUtil uses admZip, a non-web compatible dependency.

GOAL

  • Generalize project zipping functionality such that it lives in one place.
  • Remove dependency on admZip in zipUtil.
  • Increase testing coverage for the methods themselves, and for their components (by splitting them into testable components).
  • Decouple zip utility from Q agents.

Solution

  • move prepareRepoData tests to live in core module since thats where prepareRepoData lives. Do the same with zipUtil.
  • [in progress...]

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Hweinstock
Copy link
Contributor Author

/runIntegrationTests

@Hweinstock Hweinstock force-pushed the cleanUp/prepareRepoData branch from 6b83fb3 to 60638ca Compare March 20, 2025 18:28
@Hweinstock Hweinstock force-pushed the cleanUp/prepareRepoData branch from 5052c2b to 1f5c02e Compare March 20, 2025 23:44
@Hweinstock
Copy link
Contributor Author

Hweinstock commented Mar 21, 2025

/runIntegrationTests

Copy link
Contributor

@jpinkney-aws jpinkney-aws left a comment

Choose a reason for hiding this comment

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

Code is looking pretty good so far 🚀

FWIW for:

move prepareRepoData tests to live in core module since thats where prepareRepoData lives. Do the same with zipUtil.

those are probably eventually going to be moved back to amazon q anyways, once we deprecate the current implementation, since the only common thing between toolkit/q at that point will be maybe the lsp downloading?

public writeString(data: string, path: string, returnPromise: true): Promise<EntryMetaData>
public writeString(data: string, path: string, returnPromise?: false): void
public writeString(data: string, path: string, returnPromise?: boolean): void | Promise<EntryMetaData> {
const promise = this._zipWriter.add(path, new TextReader(data))
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the use case for the promise/non promise response vs just always returning a promise and then callers can either do await or void

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding of the ZipStream code is that it will wait until all files are done streaming in the finalize step. Therefore the most common use case is to avoid awaiting the promises individually so that we aren't waiting on IO operations until all files are added. Its only the tests where we await these promises.

const collectFilesOptions: CollectFilesOptions = {
maxTotalSizeBytes: this.getProjectScanPayloadSizeLimitInBytes(),
excludePatterns:
useCase === FeatureUseCase.TEST_GENERATION
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it would be hard to eventually get rid of these useCase conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, this is what I am struggling with now. These enums are everywhere in ZipUtil and make it difficult to pull out general components.

@Hweinstock Hweinstock force-pushed the cleanUp/prepareRepoData branch from 5bf0dbf to acb7361 Compare March 21, 2025 13:50
@Hweinstock
Copy link
Contributor Author

Hweinstock commented Mar 21, 2025

those are probably eventually going to be moved back to amazon q anyways, once we deprecate the current implementation,

Once the agents live in flare, it can be removed completely right?

@Hweinstock
Copy link
Contributor Author

Hweinstock commented Mar 21, 2025

Planning to scrap this work as the PR is becoming messy. The ZipUtil class needs to be heavily refactored and almost rewritten before it can be merged with prepareRepoData. The passing of use case enums makes it very difficult and risky in its current state to extract the general zipping logic. This is amplified by the lack of tests surrounding many important behaviors witnessed in this code making changes risky.

Current plan is to focus on decoupling ZipUtils from its consumers first. Once that can be done, the rest of the changes should follow more naturally and can potentially reuse ideas from the work here.

@Hweinstock Hweinstock closed this Mar 21, 2025
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