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.

Sign up for GitHub

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

Jump to bottom

Add new build flag for local-run command #959

Merged
merged 1 commit into from
May 17, 2023
Merged

Add new build flag for local-run command #959

merged 1 commit into from
May 17, 2023

Conversation

NikhilSharmaWe
Copy link
Contributor

@NikhilSharmaWe NikhilSharmaWe commented Apr 24, 2023 •
edited
Loading

Description

This PR adds a new flag build for local-run command. If the value of the build flag is set to true(default) then the function is built before local-run.

Motivation and Context

How Has This Been Tested?

I have tested the new command locally and it works as expected.

  • With --build=false, local-run command does not reflect the new changes made in the funtion.
  • With --build=true it is not the case, the changes are reflected in the run, since build command logic is being executed before each local-run.

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.

@NikhilSharmaWe NikhilSharmaWe marked this pull request as draft April 24, 2023 14:10
@NikhilSharmaWe NikhilSharmaWe marked this pull request as ready for review April 24, 2023 14:10
@alexellis
Copy link
Member

Hi, thanks for the PR.

I think what we're looking for is a --build (default of true) flag for the faas-cli local-run command.

Could you change the code and update the PR?

Alex

@alexellis
Copy link
Member

We have a community call tomorrow if you can make it? https://docs.openfaas.com/community/

@NikhilSharmaWe NikhilSharmaWe force-pushed the local-build branch 2 times, most recently from 554ca80 to 99f5a8e Compare April 25, 2023 10:23
@NikhilSharmaWe NikhilSharmaWe changed the title Add new local-build command Add new build flag for local-run command Apr 25, 2023
@NikhilSharmaWe NikhilSharmaWe force-pushed the local-build branch 3 times, most recently from c9dc19d to d570ea1 Compare April 25, 2023 10:42
alexellis
alexellis reviewed Apr 26, 2023
View reviewed changes
commands/local_run.go Outdated
cmd.Flags().IntVarP(&opts.port, "port", "p", 8080, "port to bind the function to")
cmd.Flags().StringVar(&opts.network, "network", "", "connect function to an existing network, use 'host' to access other process already running on localhost. When using this, '--port' is ignored, if you have port collisions, you may change the port using '-e port=NEW_PORT'")
cmd.Flags().StringToStringVarP(&opts.extraEnv, "env", "e", map[string]string{}, "additional environment variables (ENVVAR=VALUE), use this to experiment with different values for your function")

return cmd
}

func localBuild(cmd *cobra.Command, args []string) error {
err := preRunBuild(cmd, args)
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd probably inline these with if err:= .... ; err !=nil {

How does this work when there are flags that the build command uses like --build-arg - which are not present in the local run command's flags?

I.e. faas-cli build --build-arg GO111MODULE=on

Have a look at how faas-cli up works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made changes

@NikhilSharmaWe
add new build flag in local-run command
05c1c35 
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
@alexellis
Copy link
Member

That looks more like what I was expecting to see 👍

@NikhilSharmaWe we'll review this / try it out on the community call in 20 minutes from now.

LucasRoesler
LucasRoesler approved these changes May 6, 2023
View reviewed changes
Copy link
Member

@LucasRoesler LucasRoesler left a comment

Choose a reason for hiding this comment

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

@alexellis i just tested this locally and everything works 100% as expected. the implementation also looks good

alexellis
alexellis approved these changes May 17, 2023
View reviewed changes
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.

LGTM

Thanks for working through this with us 👍

@alexellis alexellis merged commit 2ca3f90 into openfaas:master May 17, 2023
@alexellis
Copy link
Member

This'll be available soon via https://github.com/openfaas/faas-cli/releases/tag/0.16.4

arkade get faas-cli will get the latest version once the build is complete.

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

@alexellis alexellis alexellis approved these changes

@LucasRoesler LucasRoesler LucasRoesler approved these changes

Assignees
No one assigned
Labels
new-contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: faas-cli local-run should run a build first
3 participants
@NikhilSharmaWe @alexellis @LucasRoesler