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

fix(ext/node): add chown method to FileHandle class #27638

Merged
merged 5 commits into from
Jan 15, 2025

Conversation

mstysk
Copy link
Contributor

@mstysk mstysk commented Jan 12, 2025

Add a chown method to the FileHandle class #25554.

This is my first PR. if there is anything missing, please let me know so I can fix it.

@CLAassistant
Copy link

CLAassistant commented Jan 12, 2025

CLA assistant check
All committers have signed the CLA.

@mstysk
Copy link
Contributor Author

mstysk commented Jan 12, 2025

Windows test seems to have failed

@mstysk
Copy link
Contributor Author

mstysk commented Jan 12, 2025

Similar to #27522,
chown is also untested on Windows, so I made it ignore it

@kt3k kt3k mentioned this pull request Jan 14, 2025
24 tasks
@kt3k kt3k self-requested a review January 14, 2025 07:19
Comment on lines 233 to 240
try {
await fileHandle.chown(nobodyUid, nobodyGid);
} catch (e) {
assert(
e instanceof Deno.errors.PermissionDenied,
"Expected permissionDenied error",
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you use assertRejects instead? This passes when await fileHandle.chown(nobodyUid, nobodyGid) doesn't throw

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 didn't know about assertRejects. I will take it.

@kt3k kt3k changed the title feat(node/fs): add a chown method to the FileHandle class fix(node/fs): add a chown method to the FileHandle class Jan 14, 2025
Signed-off-by: Yoshiya Hinosawa <[email protected]>
@mstysk
Copy link
Contributor Author

mstysk commented Jan 15, 2025

test release linux-x86_64 is failed. I don't understand why this test fails.

image

@kt3k
Copy link
Member

kt3k commented Jan 15, 2025

Looks like a flaky failure. I'll rerun it

@mstysk
Copy link
Contributor Author

mstysk commented Jan 15, 2025

CI is passed!

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

@kt3k kt3k merged commit b22a50c into denoland:main Jan 15, 2025
17 checks passed
@kt3k kt3k changed the title fix(node/fs): add a chown method to the FileHandle class fix(ext/node): add chown method to FileHandle class Jan 15, 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.

3 participants