X Tutup
Skip to content

feat [semver.minor]: allow seeding of files with Pods#1462

Draft
jeswr wants to merge 2 commits intoCommunitySolidServer:versions/next-majorfrom
jeswr:feat/seed-pod-data
Draft

feat [semver.minor]: allow seeding of files with Pods#1462
jeswr wants to merge 2 commits intoCommunitySolidServer:versions/next-majorfrom
jeswr:feat/seed-pod-data

Conversation

@jeswr
Copy link
Contributor

@jeswr jeswr commented Sep 26, 2022 •
edited
Loading

This is in draft status as I need to add tests. I would appreciate feedback on the API for file seeding (documented here) so that I can make any relevant changes before writing the tests.

📁 Related issues

✍️ Description

This PR allows data files to be added to Pods as part of the seeding process

✅ PR check list

Before this pull request can be merged, a core maintainer will check whether

  • this PR is labeled with the correct semver label
    • semver.patch: Backwards compatible bug fixes.
    • semver.minor: Backwards compatible feature additions.
    • semver.major: Breaking changes. This includes changing interfaces or configuration behaviour.
  • the correct branch is targeted. Patch updates can target main, other changes should target the latest versions/* branch.
  • the RELEASE_NOTES.md document in case of relevant feature or config changes.
  • any relevant documentation was updated to reflect the changes in this PR.

@jeswr jeswr marked this pull request as draft September 26, 2022 06:48
@joachimvh
Copy link
Member

joachimvh commented Sep 26, 2022

This would be nice to have but I see some issues with the current implementation.

AclHelper is a nice tool to test some things out, but would definitely need to be improved if it would be used in the actual server source. But I would solve the problem here using that tool. For one it tightly couples pod seeding to WAC authorization, which means this would not work with ACP (#1433).

CSS already has a class to copy files into a pod, that is how it fills in a pod when created: the TemplatedResourcesGenerator. See for example

CommunitySolidServer/src/init/ContainerInitializer.ts

Lines 61 to 71 in b3da9c9

const resources = this.generator.generate(this.containerId, {});
let count = 0;
for await (const { identifier: resourceId, representation } of resources) {
try {
await this.store.setRepresentation(resourceId, representation);
count += 1;
} catch (error: unknown) {
this.logger.warn(`Failed to create resource ${resourceId.path}: ${createErrorMessage(error)}`);
}
}
this.logger.info(`Initialized container ${this.containerId.path} with ${count} resources.`);

My suggestion would be to use that generator instead, and instead of setting the permissions through the JSON document require the correct authorization resources to also be in the data folder, similar to how they are in the templates/pod folder in CSS. The SeededPodInitializer would then call this class to generate the necessary resources.

One issue is that the TemplatedResourcesGenerator takes it source folder as a constructor parameter whereas in this case it differs for every call so something would need to be changed. My first thought was to have an interface that extends the ResourcesGenerator generator where the generate function takes an additional optional parameter that points to the template folder, similar to how the template engines do it internally.

@jeswr jeswr mentioned this pull request Sep 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jeswr @joachimvh
X Tutup