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 recusion to handler copy + debugPrint fn #61

Merged
merged 1 commit into from
Sep 2, 2017

Conversation

rgee0
Copy link
Contributor

@rgee0 rgee0 commented Sep 2, 2017

Signed-off-by: rgee0 richard@technologee.co.uk

Addresses issue #59.

Description

Changes the recursive arg to true on the handler copyFiles call so that sub directories within the handler are also copied into the build directory.
Extracted a debugPrint function to avoid code repetition as The new directory branch in copyFiles now messages out only if the system is creating a new directory and debug is enabled.
Corrected a typo which meant that err was being tested rather than newDirErr when a new directory is being created.
Changed FaaS to OpenFaaS where seen.

Motivation and Context

#59

  • There is a raised an issue to propose this change (required)

How Has This Been Tested?

Created 10 levels of files and folders beneath a handler 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

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: rgee0 <richard@technologee.co.uk>
@alexellis
Copy link
Member

Thanks for the fixes. If all usages are recursive then the flag is redundant and can be removed.

@alexellis
Copy link
Member

Can we get any test coverage for this scenario?


func debugPrint(message string) {

if val, exists := os.LookupEnv("debug"); exists && (val == "1" || val == "true") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could add a --debug flag for turning this on across all commands, something for a seperate PR perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was kind of the theory behind pulling it out. Suggest a separate PR since this one is a 'bug' fix

@johnmccabe
Copy link
Contributor

have built and run a few template builds locally and it looks good 👍

@alexellis
Copy link
Member

alexellis commented Sep 2, 2017

@rgee0 Merged.

Thanks for helping to integration test @johnmccabe. I would like some automated tests in "Golang" for this.. doesn't have to be super extensive. Can either of you take that on?

@alexellis alexellis merged commit f4b6ade into openfaas:master Sep 2, 2017
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