-
Notifications
You must be signed in to change notification settings - Fork 136
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
feat!: combine deploy and update commands #152
Conversation
I am planning to unify (refactor and cleanup) the rest of the commands once this PR is merged. |
} | ||
return err | ||
} | ||
} else { | ||
// Update the existing Service | ||
err = client.UpdateServiceWithRetry(encodedName, updateBuiltTimeStampEnvVar, 3) | ||
if err != nil { | ||
if !d.Verbose { | ||
err = fmt.Errorf("deployer failed to update the service: %v.\nStdOut: %s", err, output.(*bytes.Buffer).String()) | ||
} else { | ||
err = fmt.Errorf("deployer failed to update the service: %v", err) | ||
} | ||
return err | ||
} | ||
} | ||
c.SetOut(output) | ||
args := []string{ | ||
"service", "create", encodedName, | ||
"--image", f.Image, | ||
"--env", "VERBOSE=true", | ||
"--label", "bosonFunction=true", | ||
|
||
return nil | ||
} | ||
|
||
func generateNewService(name, image string) *servingv1.Service { | ||
containers := []corev1.Container{ | ||
{ | ||
Image: image, | ||
Env: []corev1.EnvVar{ | ||
{Name: "VERBOSE", Value: "true"}, | ||
}, | ||
}, | ||
} | ||
if d.Namespace != "" { | ||
args = append(args, "--namespace", d.Namespace) | ||
|
||
return &v1.Service{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: name, | ||
Labels: map[string]string{ | ||
"bosonFunction": "true", | ||
}, | ||
}, | ||
Spec: v1.ServiceSpec{ | ||
ConfigurationSpec: v1.ConfigurationSpec{ | ||
Template: v1.RevisionTemplateSpec{ | ||
Spec: v1.RevisionSpec{ | ||
PodSpec: corev1.PodSpec{ | ||
Containers: containers, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
c.SetArgs(args) | ||
err = c.Execute() | ||
if err != nil { | ||
if !d.Verbose { | ||
err = fmt.Errorf("failed to deploy the service: %v.\nStdOut: %s", err, output.(*bytes.Buffer).String()) | ||
} else { | ||
err = fmt.Errorf("failed to deploy the service: %v", err) | ||
} | ||
|
||
func updateBuiltTimeStampEnvVar(service *servingv1.Service) (*servingv1.Service, error) { | ||
envs := service.Spec.Template.Spec.Containers[0].Env | ||
|
||
builtEnvVarName := "BUILT" | ||
|
||
builtEnvVar := findEnvVar(builtEnvVarName, envs) | ||
if builtEnvVar == nil { | ||
envs = append(envs, corev1.EnvVar{Name: "VERBOSE", Value: "true"}) | ||
builtEnvVar = &envs[len(envs)-1] | ||
} | ||
|
||
builtEnvVar.Value = time.Now().Format("20060102T150405") | ||
|
||
sort.SliceStable(envs, func(i, j int) bool { | ||
return envs[i].Name <= envs[j].Name | ||
}) | ||
service.Spec.Template.Spec.Containers[0].Env = envs | ||
|
||
return service, nil | ||
} | ||
|
||
func findEnvVar(name string, envs []corev1.EnvVar) *corev1.EnvVar { | ||
var result *corev1.EnvVar = nil | ||
for i, envVar := range envs { | ||
if envVar.Name == name { | ||
result = &envs[i] | ||
break | ||
} | ||
return | ||
} | ||
// TODO: use the KN service client noted above, such that we can return the | ||
// final path/route of the final deployed Function. While it can be assumed | ||
// due to being deterministic, new users would be aided by having it echoed. | ||
return | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lance @rhuss this will let cluster to generate revision name so this: knative/serving#9544 won't affect us, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so? I'm starting to get confused about the different approaches. Is it correct to use the commands or the client as in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My impression was, that we should use the client, but I might be wrong.
@rhuss wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend using the client APIs. However not the commands but the client API which comes as an interface as here --> https://github.com/knative/client/blob/master/pkg/serving/v1/client.go (this is not the serving API but a facade to it)
Please don't use the commands. They can change anytime in incompatible ways, there are no precautions to prevent this.
Also we are planning to expose the client API (not the commands!), the way how to parse options, how to parse sink prefixes, ... and generally anything that can be useful for implementing an plugin into a separate repository (knative/client-pkg
as working title), so that you don't even have a direct dependency to knative/client
anymore (which then also prevents you technically to access the cobra kn commands)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this and K_SINK is injected, unlike before this change 👍 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! A few minor comments. And one big one about client vs command in kn.
deployCmd.Flags().StringP("namespace", "n", "", "Override namespace into which the Function is deployed (on supported platforms). Default is to use currently active underlying platform setting - $FAAS_NAMESPACE") | ||
deployCmd.Flags().StringP("path", "p", cwd(), "Path to the function project directory - $FAAS_PATH") | ||
deployCmd.Flags().StringP("repository", "r", "", "Repository for built images, ex 'docker.io/myuser' or just 'myuser'. - $FAAS_REPOSITORY") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`, | ||
SuggestFor: []string{"delpoy", "deplyo"}, | ||
PreRunE: bindEnv("namespace", "path", "confirm"), | ||
PreRunE: bindEnv("image", "namespace", "path", "repository", "confirm"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting that this is affected by #156 as well.
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"knative.dev/client/pkg/wait" | ||
servingv1 "knative.dev/serving/pkg/apis/serving/v1" | ||
v1 "knative.dev/serving/pkg/apis/serving/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You and @dsimansk are taking different approaches with this file. I know that you are also dealing with a lot more logic, but I wonder which is right - using the client or using a command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have briefly discussed with @rhuss that it might be beneficial to avoid direct usage of serving/eventing client and rather go with kn abstraction.
For this particular case it'd mean a lot less mangling with service object directly. And in addition we can expect that some level of test coverage is done on kn side already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsimansk btw, in this PR we are not using serving/eventing client, but the one provided by knative/client: https://github.com/boson-project/faas/pull/152/files#diff-0cff654ab7f6b7619dd3ebd4a8855f70R11-R37
But I suppose that doesn't change anything from your suggestion, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly recommend choosing either/or. However, if you use the kn
API that is offered in front of eventing/serving you still have to import serving/eventing APIs like this, since the signature of the kn client API references the serving objects directly.
For consistencies sake and from an architectural POV though I would definitely recommend to use the client API if it offers all that you need. If not, we should fix this in the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhuss I am starting to be confused a little bit, sorry for that, but I am getting lost in the nomenclature, since the word client
has multiple meanings in this context 😅
So your suggestion is to use go client provided byt knative/client? eg:
client, err := p.NewServingClient(namespace)
...
client.CreateService(service *servingv1.Service)
or to use the kn abstraction as @dsimansk mentioned? eg:
c := service.NewServiceCreateCommand(¶ms)
args := []string{
encodedName,
"--image", f.Image,
"--env", "VERBOSE=true",
"--label", "bosonFunction=true",
}
c.SetArgs(args)
err = c.Execute()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The former. No reference to any cobra commands, but just the services. There is also a better way to create an instance of the service that does not goes over the parameter as in the example above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly to avoid cyclic references, and keep a hierarchical dependency tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, that's was my original understanding of this.
} | ||
return err | ||
} | ||
} else { | ||
// Update the existing Service | ||
err = client.UpdateServiceWithRetry(encodedName, updateBuiltTimeStampEnvVar, 3) | ||
if err != nil { | ||
if !d.Verbose { | ||
err = fmt.Errorf("deployer failed to update the service: %v.\nStdOut: %s", err, output.(*bytes.Buffer).String()) | ||
} else { | ||
err = fmt.Errorf("deployer failed to update the service: %v", err) | ||
} | ||
return err | ||
} | ||
} | ||
c.SetOut(output) | ||
args := []string{ | ||
"service", "create", encodedName, | ||
"--image", f.Image, | ||
"--env", "VERBOSE=true", | ||
"--label", "bosonFunction=true", | ||
|
||
return nil | ||
} | ||
|
||
func generateNewService(name, image string) *servingv1.Service { | ||
containers := []corev1.Container{ | ||
{ | ||
Image: image, | ||
Env: []corev1.EnvVar{ | ||
{Name: "VERBOSE", Value: "true"}, | ||
}, | ||
}, | ||
} | ||
if d.Namespace != "" { | ||
args = append(args, "--namespace", d.Namespace) | ||
|
||
return &v1.Service{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: name, | ||
Labels: map[string]string{ | ||
"bosonFunction": "true", | ||
}, | ||
}, | ||
Spec: v1.ServiceSpec{ | ||
ConfigurationSpec: v1.ConfigurationSpec{ | ||
Template: v1.RevisionTemplateSpec{ | ||
Spec: v1.RevisionSpec{ | ||
PodSpec: corev1.PodSpec{ | ||
Containers: containers, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
c.SetArgs(args) | ||
err = c.Execute() | ||
if err != nil { | ||
if !d.Verbose { | ||
err = fmt.Errorf("failed to deploy the service: %v.\nStdOut: %s", err, output.(*bytes.Buffer).String()) | ||
} else { | ||
err = fmt.Errorf("failed to deploy the service: %v", err) | ||
} | ||
|
||
func updateBuiltTimeStampEnvVar(service *servingv1.Service) (*servingv1.Service, error) { | ||
envs := service.Spec.Template.Spec.Containers[0].Env | ||
|
||
builtEnvVarName := "BUILT" | ||
|
||
builtEnvVar := findEnvVar(builtEnvVarName, envs) | ||
if builtEnvVar == nil { | ||
envs = append(envs, corev1.EnvVar{Name: "VERBOSE", Value: "true"}) | ||
builtEnvVar = &envs[len(envs)-1] | ||
} | ||
|
||
builtEnvVar.Value = time.Now().Format("20060102T150405") | ||
|
||
sort.SliceStable(envs, func(i, j int) bool { | ||
return envs[i].Name <= envs[j].Name | ||
}) | ||
service.Spec.Template.Spec.Containers[0].Env = envs | ||
|
||
return service, nil | ||
} | ||
|
||
func findEnvVar(name string, envs []corev1.EnvVar) *corev1.EnvVar { | ||
var result *corev1.EnvVar = nil | ||
for i, envVar := range envs { | ||
if envVar.Name == name { | ||
result = &envs[i] | ||
break | ||
} | ||
return | ||
} | ||
// TODO: use the KN service client noted above, such that we can return the | ||
// final path/route of the final deployed Function. While it can be assumed | ||
// due to being deterministic, new users would be aided by having it echoed. | ||
return | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so? I'm starting to get confused about the different approaches. Is it correct to use the commands or the client as in this case?
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
Co-authored-by: Lance Ball <lball@redhat.com>
Co-authored-by: Lance Ball <lball@redhat.com>
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
7e2a57b
to
dc99a11
Compare
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
Deploys the Function project in the current directory. A path to the project | ||
directory may be provided using the --path or -p flag. The image to be deployed | ||
must have already been created using the "build" command. | ||
Builds and Deploys the Function project in the current directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this description it looks like that faas build
and faas deploy
overlap, so that deploy
is a superset of build
. This is fine, but then it would be better to include the build only mode as an option to faas deploy
, like in faas deploy --build-only
. This would have several benefits:
- Reducing duplication in options that are the same for
deploy
andbuild
- Less commands, smaller UI interface
Drawbacks are:
- Maybe not easy to find out which options of
faas deploy
are relevant for build and which for creating the service. - Build can be considered as a high-level concern, so a dedicated command might reflect this more prominently
I'm not sure what's the best way to go. From my experience with serverless.com
integration, I can say that "Faas" platforms don't expose the "build" aspect so prominently as many FaaS platform don't have that stage anyway (think AWS Lambda). So it's just an aspect of the one single, most important operation: How to get my local code running in the cloud ? Aka "deploy". From that angle, I'd recommend to fold "build" as just another aspect of "deploy" and enable this with a dedicated option that influence the flow (its a bit similar to --dry-run
option that is used by many commands)
On the other hand, if we really want to expose the build as a first level concept, which e.g. is also used without deploying (eg for local development with containers), then this separate commands make definitely sense.
@lance @matejvasek Is this ready to be merged? So we can move forward with the follow up PRs, that need to be rebased. |
Based on our call today, yes I think so. |
Signed-off-by: Zbynek Roubalik zroubali@redhat.com
faas deploy
now builds and deploys a Function.If the Function is already deployed, it is updated.
Fixes: #135