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.

Sign up for GitHub

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

Jump to bottom

fix issue #12657 #12658

Merged
merged 2 commits into from
Sep 30, 2024
Merged

fix issue #12657 #12658

merged 2 commits into from
Sep 30, 2024

Conversation

redjumper
Copy link
Contributor

修复issue #12657
pr #12596 对uid的判断有问题，还有漏了/siyuan/workspace的文件权限

我这边测试没问题

@github-actions github-actions bot changed the base branch from master to dev September 30, 2024 03:02
@github-actions GitHub Actions
Copy link

Your PR was set to target master, PRs should be target dev
The base branch of this PR has been automatically changed to dev, please check that there are no merge conflicts

@redjumper redjumper mentioned this pull request Sep 30, 2024
Closed
3 tasks
@88250
Copy link
Member

88250 commented Sep 30, 2024

@Macavity Please review it

@redjumper
Copy link
Contributor Author

redjumper commented Sep 30, 2024 •
edited
Loading

如果指定了workspace，相应地得改workspace的所有者权限，而不是/siyuan/workspace

@88250
Copy link
Member

88250 commented Sep 30, 2024

如果指定了workspace，相应地得改workspace的所有者权限，而不是/siyuan/workspace

这个 PR 中没有实现对吗？只是写死了 /siyuan/workspace 路径？

@redjumper
Copy link
Contributor Author

这个 PR 中没有实现对吗？只是写死了 /siyuan/workspace 路径？

没有实现，看Macavity的pr才看到有workspace参数。我修改的话暂时只想到引入env，如果这样修改，以前指定了workspace参数的得修改运行命令。等看看其它人有什么优雅的实现吧

Macavity
Macavity approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@Macavity Macavity left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me.

@redjumper
Copy link
Contributor Author

redjumper commented Sep 30, 2024 •
edited
Loading

@Macavity I believe that the ownership of the workspace should also change if a specific workspace is mentioned. This pr implements that change following your initial review. Please take a moment to review it again.

@redjumper
Copy link
Contributor Author

如果指定了workspace，相应地得改workspace的所有者权限，而不是/siyuan/workspace

这个 PR 中没有实现对吗？只是写死了 /siyuan/workspace 路径？

已经实现

Macavity
Macavity reviewed Sep 30, 2024
View reviewed changes
kernel/entrypoint.sh
chown -R "${PUID}:${PGID}" /opt/siyuan
chown -R "${PUID}:${PGID}" /home/siyuan/
chown -R "${PUID}:${PGID}" /siyuan/workspace
chown -R "${PUID}:${PGID}" "${WORKSPACE_DIR}"
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good idea 👍

Macavity
Macavity approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@Macavity Macavity left a comment

Choose a reason for hiding this comment

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

Thank you for adjusting the original changes and allow to adjust the workspace directory ownership as well 👍

@88250 88250 merged commit 4ef5d90 into siyuan-note:dev Sep 30, 2024
@88250
Copy link
Member

88250 commented Sep 30, 2024

Thanks to everyone for discussing and solving this issue, we merged it.

@88250 88250 mentioned this pull request Sep 30, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Macavity Macavity Macavity approved these changes

Assignees

@redjumper redjumper

Labels
Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@redjumper @88250 @Macavity