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(cli): create Zip from project #235

Merged
merged 39 commits into from
Jun 3, 2021
Merged

feat(cli): create Zip from project #235

merged 39 commits into from
Jun 3, 2021

Conversation

y-lakhdar
Copy link
Contributor

@y-lakhdar y-lakhdar commented May 28, 2021

Proposed changes

Reads the project, compress the resources into a single zip and use it to create a snapshot.
The command uses a third party library to compress the resources. I initially tried using the native zlib API but it seems to only support single file compression. We can probably achieve the same result with some stream manipulation but, I am keeping it simple for now.

Testing

  • Unit Tests:
  • Manual Tests: Created a project using this structure.

CDX-353

@github-actions
Copy link
Contributor

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

did an early sneak peak between two builds ;)

@y-lakhdar y-lakhdar marked this pull request as ready for review May 29, 2021 13:40
@y-lakhdar y-lakhdar requested a review from olamothe as a code owner May 29, 2021 13:40
@y-lakhdar y-lakhdar changed the base branch from master to CDX-360 May 30, 2021 14:56
Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

Some changes in the mocks :)

@y-lakhdar y-lakhdar requested a review from louis-bompart June 1, 2021 01:41
Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

Suggestion to streamline and avoid casting in the mocks

Comment on lines 26 to 36
const doMockAuthenticatedClient = () => {
mockedAuthenticatedClient.mockImplementation(
() =>
({
getClient: () =>
Promise.resolve({
resourceSnapshot: {createFromFile: mockedCreateSnapshotFromFile},
}),
} as unknown as AuthenticatedClient)
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const doMockAuthenticatedClient = () => {
mockedAuthenticatedClient.mockImplementation(
() =>
({
getClient: () =>
Promise.resolve({
resourceSnapshot: {createFromFile: mockedCreateSnapshotFromFile},
}),
} as unknown as AuthenticatedClient)
);
};
const doMockAuthenticatedClient = () => {
mockedAuthenticatedClient.prototype.getClient.mockImplementation(() =>
Promise.resolve({
resourceSnapshot: {createFromFile: mockedCreateSnapshotFromFile},
})
);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it still causes the same error. Now the resourceSnapshot mock is missing all its methods.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm yeah, the PlatformClient was missing some stuff.
Double check but I think this would work tho

Suggested change
const doMockAuthenticatedClient = () => {
mockedAuthenticatedClient.mockImplementation(
() =>
({
getClient: () =>
Promise.resolve({
resourceSnapshot: {createFromFile: mockedCreateSnapshotFromFile},
}),
} as unknown as AuthenticatedClient)
);
};
const doMockAuthenticatedClient = () =>
mockedAuthenticatedClient.prototype.getClient.mockImplementation(() =>
Promise.resolve<PlatformClient>({
resourceSnapshot: {createFromFile: mockedCreateSnapshotFromFile},
} as unknown as PlatformClient)
);

@y-lakhdar y-lakhdar changed the base branch from CDX-360 to master June 1, 2021 18:37
Comment on lines 26 to 36
const doMockAuthenticatedClient = () => {
mockedAuthenticatedClient.mockImplementation(
() =>
({
getClient: () =>
Promise.resolve({
resourceSnapshot: {createFromFile: mockedCreateSnapshotFromFile},
}),
} as unknown as AuthenticatedClient)
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm yeah, the PlatformClient was missing some stuff.
Double check but I think this would work tho

Suggested change
const doMockAuthenticatedClient = () => {
mockedAuthenticatedClient.mockImplementation(
() =>
({
getClient: () =>
Promise.resolve({
resourceSnapshot: {createFromFile: mockedCreateSnapshotFromFile},
}),
} as unknown as AuthenticatedClient)
);
};
const doMockAuthenticatedClient = () =>
mockedAuthenticatedClient.prototype.getClient.mockImplementation(() =>
Promise.resolve<PlatformClient>({
resourceSnapshot: {createFromFile: mockedCreateSnapshotFromFile},
} as unknown as PlatformClient)
);

@y-lakhdar y-lakhdar requested a review from louis-bompart June 2, 2021 11:56
Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

I think it'd be more syntactically correct to do a default import ;)

import {existsSync, createWriteStream, WriteStream, unlinkSync} from 'fs';
import {Project} from './project';
import {join} from 'path';
import * as archiver from 'archiver';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mh, I would enable esModuleInterop in the tsconfig and do

Suggested change
import * as archiver from 'archiver';
import archiver, {Archiver} from 'archiver';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it will work in this case since there are no default export in the archiver package

Copy link
Collaborator

@louis-bompart louis-bompart Jun 2, 2021

Choose a reason for hiding this comment

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

image
Tested on my machine, it works ;)
UT green too
image
You need the three suggestions tho ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oups, I updated the wrong tsconfig 🤦

pipe: mockedPipe,
directory: mockedPassDirectory,
finalize: mockedFinalize,
} as unknown as archiver.Archiver)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you do my suggestion above

Suggested change
} as unknown as archiver.Archiver)
} as unknown as Archiver)

import {createWriteStream, existsSync, unlinkSync} from 'fs';
import {join} from 'path';
import {cli} from 'cli-ux';
import * as archiver from 'archiver';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import * as archiver from 'archiver';
import archiver from 'archiver';

@y-lakhdar y-lakhdar merged commit 3ab41c8 into master Jun 3, 2021
@louis-bompart louis-bompart deleted the CDX-353 branch July 29, 2021 16:48
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