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

Default to stack.yml when no --yaml flag given #126

Merged
merged 2 commits into from Nov 6, 2017

Conversation

Projects
None yet
5 participants
@nicholasjackson
Contributor

nicholasjackson commented Sep 26, 2017

Description

Added capability to load a default configuration file stack.yaml when present in the current folder. If a default is present this can still be overridden by providing the flag --yaml=file.yaml

Motivation and Context

  • I have raised an issue to propose this change (required)

How Has This Been Tested?

Code has been tested on a unit level and basic manual testing has also been performed.

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.
@open-derek

This comment has been minimized.

Show comment
Hide comment
@open-derek

open-derek bot Sep 26, 2017

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

open-derek bot commented Sep 26, 2017

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off.
That's something we need before your Pull Request can be merged. Please see our contributing guide.

@open-derek open-derek bot added the no-dco label Sep 26, 2017

Show outdated Hide outdated README.md Outdated
Show outdated Hide outdated commands/faas_test.go Outdated
Show outdated Hide outdated commands/faas_test.go Outdated
Show outdated Hide outdated commands/faas.go Outdated
Show outdated Hide outdated commands/faas_test.go Outdated
Show outdated Hide outdated commands/faas_test.go Outdated
@alexellis

Looks pretty close a this point. I think we could use the folder: tests like in the other projects for the Golang test files and then if you could sign-off all the individual commits Derek will stop complaining too.

Easiest way to do that sometimes is to reset HEAD~N (N=commit count) - add the files again, commit with -s and then push with force.

@alexellis alexellis added this to Todo in #TeamServerless Sep 27, 2017

Show outdated Hide outdated commands/faas.go Outdated

@open-derek open-derek bot removed the no-dco label Sep 28, 2017

@nicholasjackson

This comment has been minimized.

Show comment
Hide comment
@nicholasjackson

nicholasjackson Sep 28, 2017

Contributor

I think all the requested changes have been completed, I have also signed off the commit hopefully all good

Contributor

nicholasjackson commented Sep 28, 2017

I think all the requested changes have been completed, I have also signed off the commit hopefully all good

Show outdated Hide outdated commands/faas_test.go Outdated
@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Sep 30, 2017

Member

Good to get the test with this 👍 I added a comment around just faking the os.Stat and returning a canned value.

Member

alexellis commented Sep 30, 2017

Good to get the test with this 👍 I added a comment around just faking the os.Stat and returning a canned value.

@alexellis

@nicholasjackson this is in a state now where it could be merged. I would like to avoid writing to the filesystem doing the tests if possible. When I wrote the tests for the config reading code I abstracted Getenv behind an interface. We could apply the same here then merge this PR - what do you think?

@alexellis alexellis changed the title from Default yaml to Default to stack.yml when no --yaml flag given Oct 1, 2017

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Oct 1, 2017

Member

This tests out OK for me. Now that we're building an experience around a default .yml file - should we think about changing the new functionality to write/append to "stack.yml" by default? cc/ @ericstoekl @rgee0 @johnmccabe

Member

alexellis commented Oct 1, 2017

This tests out OK for me. Now that we're building an experience around a default .yml file - should we think about changing the new functionality to write/append to "stack.yml" by default? cc/ @ericstoekl @rgee0 @johnmccabe

@rgee0

This comment has been minimized.

Show comment
Hide comment
@rgee0

rgee0 Oct 1, 2017

Member

@alexellis I was thinking along those lines after reviewing @ericstoekl's work on the append functionality. I think it would make sense, and result in a better experience to default it and allow over-rides.

Member

rgee0 commented Oct 1, 2017

@alexellis I was thinking along those lines after reviewing @ericstoekl's work on the append functionality. I think it would make sense, and result in a better experience to default it and allow over-rides.

nicholasjackson added some commits Sep 26, 2017

Added capability to load default yaml stack.yaml
Signed-off-by: Nic Jackson <jackson.nic@gmail.com>
Updated test to use faked os.Stat
Signed-off-by: Nic Jackson <jackson.nic@gmail.com>
@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Oct 23, 2017

Member

@nicholasjackson is this good to go? Anything else needed?

Member

alexellis commented Oct 23, 2017

@nicholasjackson is this good to go? Anything else needed?

@alexellis

This comment has been minimized.

Show comment
Hide comment
@alexellis

alexellis Oct 23, 2017

Member

@itscaro can you test?

Member

alexellis commented Oct 23, 2017

@itscaro can you test?

faasCmd.SilenceUsage = true
faasCmd.SetArgs(customArgs[1:])
faasCmd.Execute()
}
func checkAndSetDefaultYaml() {

This comment has been minimized.

@alexellis

alexellis Nov 2, 2017

Member

I want to get the fix in, think this is going to be a good addition. This method feels hacky to do this to a global variable. @johnmccabe @ericstoekl @nicholasjackson any other suggestions?

@alexellis

alexellis Nov 2, 2017

Member

I want to get the fix in, think this is going to be a good addition. This method feels hacky to do this to a global variable. @johnmccabe @ericstoekl @nicholasjackson any other suggestions?

@alexellis alexellis merged commit 572515c into openfaas:master Nov 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alexellis alexellis moved this from In review to Read to merge in #TeamServerless Nov 6, 2017

@alexellis alexellis moved this from Read to merge to Done in #TeamServerless Nov 6, 2017

@alexellis alexellis removed this from Done in #TeamServerless Mar 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment