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 [semver.minor]: allow seeding of files with Pods #1462

Draft
wants to merge 2 commits into
base: versions/next-major
Choose a base branch
from

Conversation

jeswr
Copy link
Contributor

@jeswr jeswr commented Sep 26, 2022

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

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

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.

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.

None yet

2 participants