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

Added copyTree function and replaced the template copyFiles calls #21

Merged
merged 1 commit into from
Jul 24, 2017

Conversation

rgee0
Copy link
Contributor

@rgee0 rgee0 commented Jul 23, 2017

Signed-off-by: rgee0 [email protected]

Main Change:
Created a copyTree function to address issue#18.
The function takes a similar approach to copyFiles but on finding a folder creates an equivalent folder in the build dir and then calls itself, this time passing in the source folder and newly created target folder. This means that all files and folder to n-levels will be copied.
A helper function has also been created; pathExists() is used within copyTree to try to offset errors where the destination may already exist.
Finally, replaced the two copyFile calls for the template folders with a single call to copyTree and amended the TODO comment accordingly.

Test:
Created 10 levels of files and folders beneath a template folder
Ran ./faas-cli -action build -yaml ./samples.yml
Confirmed that the images built successfully.
Confirmed that the expected files and folders were extant in the build folder following completion of build.

Other small edits:
Standardised some of the messages returned to the user.
Some diffs being shown are a result of vscode re-formatting.

Misc:
Used git commit -s as per the contribution guide

@alexellis
Copy link
Member

Thanks for this!

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Looks good. I wonder if we can keep the name copyFiles or something similar?

@rgee0
Copy link
Contributor Author

rgee0 commented Jul 24, 2017

I left copyFiles in for situations where only files needed copying, so we have a choice:

Assume every copy will need to do the whole tree from its start path. Remove copyFiles and rename copyTreeTo copyFiles.

Leave copyFiles for situations where only the path's files require to be copied. Rename copyTree To copyFilesTree.

@alexellis
Copy link
Member

alexellis commented Jul 24, 2017

I was thinking a single function - either copyFiles which is always depth-recursive or copyFiles plus a flag recursive for when you don't want to go down the file-structure?

@rgee0
Copy link
Contributor Author

rgee0 commented Jul 24, 2017

Cool. We'll go with the additional param since that allows us greater control

@rgee0 rgee0 force-pushed the traverseFolders branch from 444aad3 to 5c37157 Compare July 24, 2017 16:51
@alexellis alexellis merged commit 3ff2367 into openfaas:master Jul 24, 2017
@alexellis
Copy link
Member

Thanks for this 💥

@rgee0 rgee0 deleted the traverseFolders branch July 26, 2017 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants