Remove language default and fix command flags #170
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. |
|
@rgee0 this should fix the bug you'd spotted when looking at openfaas/faas#297 |
rgee0
reviewed
Oct 15, 2017
commands/deploy.go
Outdated
| @@ -159,6 +159,7 @@ func runDeploy(cmd *cobra.Command, args []string) { | |||
| os.Exit(1) | |||
| } | |||
|
|
|||
| fmt.Printf("GOING IN: %s, %s, %s, %s, %s\n", fprocess, gateway, functionName, image, language) | |||
There was a problem hiding this comment.
Ah I saw it come up in the git diff, but it wasn't there in the editor pane, assumed VSCode was playing up - but yep, will nuke.
|
I’ll try to give it a run out tomorrow |
This commit stops using `node` as a default language due to it conflicting with functions where the `fprocess` is set in the image. Given the large numbers of languages having a default makes less sense now. It also removes some of the copy-paste/redundant/irrelevant flag definitions that ended up in newly added subcommands. Signed-off-by: John McCabe <[email protected]>
|
Confirming this fixes the problem, thanks John! |
alexellis
reviewed
Oct 19, 2017
|
|
||
| ``` | ||
| $ faas-cli build \ | ||
| --image=alexellis2/node_info \ | ||
| --lang= node \ |
There was a problem hiding this comment.
@johnmccabe can you push this fixed? ^ extra space between = and node
alexellis
reviewed
Oct 19, 2017
| @@ -26,7 +26,7 @@ func init() { | |||
| buildCmd.Flags().StringVar(&image, "image", "", "Docker image name to build") | |||
| buildCmd.Flags().StringVar(&handler, "handler", "", "Directory with handler for function, e.g. handler.js") | |||
| buildCmd.Flags().StringVar(&functionName, "name", "", "Name of the deployed function") | |||
| buildCmd.Flags().StringVar(&language, "lang", "node", "Programming language template") | |||
| buildCmd.Flags().StringVar(&language, "lang", "", "Programming language template") | |||
alexellis
reviewed
Oct 19, 2017
| @@ -15,18 +15,15 @@ import ( | |||
| ) | |||
|
|
|||
| var ( | |||
| verboseInvoke bool | |||
There was a problem hiding this comment.
What happened to verboseInvoke?
alexellis
reviewed
Oct 19, 2017
| @@ -18,12 +18,7 @@ var ( | |||
|
|
|||
| func init() { | |||
| // Setup flags that are used by multiple commands (variables defined in faas.go) | |||
| listCmd.Flags().StringVar(&fprocess, "fprocess", "", "Fprocess to be run by the watchdog") | |||
alexellis
requested changes
Oct 19, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR stops using
nodeas a default language due to it conflicting with functions where thefprocessenvvar is set in the image.Given the large numbers of languages having a default makes less sense now.
It also removes some of the copy-paste/redundant/irrelevant flag definitions that ended up in newly added subcommands.
Motivation and Context
node index.json deploy by default, ignoring the value set in images #169, also Question about using AlpineFunction sample function faas#297)How Has This Been Tested?
--fprocessflag, function gets created with one being set as expected and correctly honours the setting in the image.Types of changes
Strictly speaking this is a breaking change, but it only impacts a very small edge case - that is where you deploy node functions via the CLI and rely on the language being set to
nodeby default.As mentioned above, the growing number of function languages means that for the vast majority of users this won't be an issue and I think we're better having a single common behaviour for all users - which for the moment should include setting the language flag
--lang.Note that the logic setting a default
fprocesswhen a language is set has not changed and remains useful.Checklist:
git commit -s