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

Create directory structure when writing files #244

Merged
merged 4 commits into from
Oct 6, 2023

Conversation

agyoungs
Copy link
Contributor

This simple change will allow an extension to have a directory structure for the files to copy into the container. This can simplify structuring the directory in the container if you simply wish to preserve the structure from the host machine.

@agyoungs agyoungs requested a review from tfoote as a code owner September 25, 2023 02:18
Copy link
Collaborator

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

This seems reasonable. This took longer to understand as looks like you made a few unnecessary changes and it only needs to be the one line change in L334.

Can you also add some tests to cover different cases with this as well as a proposed documentation update too?

@agyoungs
Copy link
Contributor Author

agyoungs commented Oct 2, 2023

This seems reasonable. This took longer to understand as looks like you made a few unnecessary changes and it only needs to be the one line change in L334.

Can you also add some tests to cover different cases with this as well as a proposed documentation update too?

Yeah, I renamed file_name to filepath because the specified location can now be a path (before, it would fail if it was a path).

I'll try to add some tests in the next day or two when I get a chance.

@agyoungs
Copy link
Contributor Author

agyoungs commented Oct 6, 2023

I forgot to come back to this until now, but let me know if you think the additional test is sufficient. I also added an explicit check for paths outside the temp directory.

Copy link
Collaborator

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests.

@tfoote tfoote merged commit c62c4b9 into osrf:main Oct 6, 2023
2 checks passed
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.

2 participants