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

Add support for mounting custom volumes (cloud credentials) #212

Merged
merged 3 commits into from
Jun 19, 2023

Conversation

GeorgesLorre
Copy link
Collaborator

No description provided.

@GeorgesLorre GeorgesLorre marked this pull request as ready for review June 15, 2023 13:29
@ChristiaensBert
Copy link
Contributor

There seem to be a lot of file changes with formatting / renaming. Is this from the Pandas PR?

@GeorgesLorre
Copy link
Collaborator Author

There seem to be a lot of file changes with formatting / renaming. Is this from the Pandas PR?

should be solved (had to rebase)

Base automatically changed from feature/service-build to main June 15, 2023 14:53
Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks @GeorgesLorre! Some small comments.

docs/pipeline.md Outdated Show resolved Hide resolved
docs/pipeline.md Outdated Show resolved Hide resolved
# example for GCP, this mounts your application default credentials to the docker container
"$HOME/.config/gcloud/application_default_credentials.json:/root/.config/gcloud/application_default_credentials.json:ro"
# exaple for AWS, this mounts your current credentials to the docker container
"$HOME/.aws/credentials:/root/.aws/credentials:ro"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe check with the Azure heroes if there's a similar way to it for Azure?

@@ -106,7 +117,7 @@ def _generate_spec(self, pipeline: Pipeline) -> dict:
command = ["--metadata", json.dumps(asdict(metadata))]

# add in and out manifest paths to command
command.extend(["--output_manifest_path", f"{path}/manifest.txt"])
command.extend(["--output_manifest_path", f"{path}/manifest.json"])
Copy link
Member

Choose a reason for hiding this comment

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

This still overwrites the manifest for every component. We should still address this, but can be taken up in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will fix in another PR

fondant/pipeline.py Outdated Show resolved Hide resolved

extra_volumes = [
# example for GCP, this mounts your application default credentials to the docker container
"$HOME/.config/gcloud/application_default_credentials.json:/root/.config/gcloud/application_default_credentials.json:ro"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the user know the correct within the container themselves or should we provide some additional docs on it. When using an SA, is the application_default_credentials.json still a correct path to mount to?

docs/pipeline.md Outdated
extra_volumes = [
# example for GCP, this mounts your application default credentials to the docker container
"$HOME/.config/gcloud/application_default_credentials.json:/root/.config/gcloud/application_default_credentials.json:ro"
# exaple for AWS, this mounts your current credentials to the docker container
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# exaple for AWS, this mounts your current credentials to the docker container
# example for AWS, this mounts your current credentials to the docker container

Args:
pipeline: the pipeline to compile
output_path: the path where to save the docker-compose spec
extra_volumes: a list of extra volumes (using the Short syntax:
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some information about the format of the extra_volumes. e.g. list of "source:target" strings with absolute paths where target is the credentials folder/file path specified by the used cloud provider/service

@GeorgesLorre GeorgesLorre merged commit dc455e2 into main Jun 19, 2023
@GeorgesLorre GeorgesLorre deleted the feature/mount-extra-volumes branch June 19, 2023 07:29
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