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

Use unzip.Parse over unzip.Extract #1666

Merged
merged 20 commits into from
Feb 23, 2024
Merged

Use unzip.Parse over unzip.Extract #1666

merged 20 commits into from
Feb 23, 2024

Conversation

bethanyj28
Copy link
Contributor

@bethanyj28 bethanyj28 commented Feb 22, 2024

Updating our unzipping logic to use unzip.Parse and handle the entry event for greater control of the unzip process.

@bethanyj28 bethanyj28 marked this pull request as ready for review February 23, 2024 13:52
@bethanyj28 bethanyj28 requested review from a team as code owners February 23, 2024 13:52
new stream.Transform({
objectMode: true,
transform: async (entry, _, callback) => {
const fullPath = path.normalize(path.join(directory, entry.path))
Copy link
Member

Choose a reason for hiding this comment

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

const fullPath = path.resolve(directory, entry.path)

will path.resolve() be an better option, assume entry.path should be a relative path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me! I think download-artifact ensures that the path is absolute, but it doesn't look like toolkit does any validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, doing that throws off the check below for the directory path 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you mind if I changed it back? On second thought, it might be better to keep this generic and not assume path for anyone utilizing this package. For download-artifact, we have the absolute path: https://github.com/actions/download-artifact/blob/348754975ef0295bfa2c111cba996120cfdf8a5d/src/download-artifact.ts#L38

Copy link
Member

Choose a reason for hiding this comment

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

i thought directory is absolute path, looks like it can be relative...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Directory is absolute when it comes from download-artifact, but isn't guaranteed absolute from other sources.

Co-authored-by: Tingluo Huang <tingluohuang@github.com>
objectMode: true,
transform: async (entry, _, callback) => {
const fullPath = path.normalize(path.join(directory, entry.path))
if (!directory.endsWith(path.sep)) {
Copy link

Choose a reason for hiding this comment

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

It is important for the comparison below. Otherwise it will fail with, for example, "/tmp/dest/file".startsWith("../dest/")

@bethanyj28 bethanyj28 merged commit 88f7a7b into main Feb 23, 2024
14 checks passed
@bethanyj28 bethanyj28 deleted the bethanyj28/download-path branch February 23, 2024 21:22
Copy link

@gdamiaN538 gdamiaN538 left a comment

Choose a reason for hiding this comment

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

[gh]

@gdamiaN538
Copy link

gdamiaN538 commented Mar 1, 2024 via email

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.

8 participants