feat: add init/build/deploy commands and customizable namespace #65
Conversation
|
Review requested - but note that it is still a WIP |
|
One thing that seems a little inconsistent is how configuration parameters are dealt with. Specifically, I mean for example, something like |
There was a problem hiding this comment.
A few things changed in this PR simultaneously, so yes it will be a little bit of a challenge to get everything working, but we can do it! Let's communicate via Slack and I can explain how those function mocks for tests work, and we can discuss any other gotchas too.
This commit comprises some fairly large changes in the codebase. The 'create' command has been extracted into 'init', 'bulid' and 'deploy' commands. The 'create' command remains, but now delegates most of its work to these other three. This also has resulted in some rework of the various flags. One example is the removal of the '--local' flag from create, since this can be achieved now by just running 'faas init'. There are still a number of changes needed for this to land. * I've removed some of the 'internal' stub code * The environment variables have been removed (for the most part) from --help text and should be returned * The progress meter is a little wonky now * The service address is incorrect when printed * There are no tests, and I'm pretty sure I've broken what tests already existed It's a start - a work in progress.
lance
force-pushed
the
lance/62-configurable-namespace
branch
from
83deabb
to
5b84c59
Compare
|
@lkingland all tests and lint checks are passing. PTAL |
|
Looking great @lance, glad that snapped into place. Doing a bit more in depth looksee now... |
There was a problem hiding this comment.
This really did end up touching all sorts of things. Looks great so far!
I am pretty sure we will still have a few things to iron out when integration testing, such as upgrading to the knative.dev client such that we get the actual route echoed back, and ensuring that it actually pushes (the call to Push appears to still be a WIP). But this sure looks good enough for me to rubber-stamp an LGTM
cmd/build.go
Outdated
| // uses this to load a faas.Function. If the config contains | ||
| // a value for the image tag, this will be used and | ||
| // subsequently written to the config file. | ||
| func FunctionConfigForBuild(config buildConfig) (f *faas.Function, err error) { |
There was a problem hiding this comment.
There's some logic in here in FunctionConfigForBuild and Build that is not really CLI-specific
- If Name and Runtime are not provided, generate an error
- If a Tag is not provided, use the tag from the config file in the function root
- Tags default to quay.io/%s:latest
- Update the Function config file on disk at function root
- Optionally push on Build
These logic bits are simple, yes, but they are applicable to a developer using the client library directly or the CLI. So perhaps this is a job for a future refactor, but it would be nice to see these moved into the Client Library, and the API used here in the CLI kept to a very simple "map in booleans and strings, then invoke Build". This would both remove the need to do a conditional instantiation of the client library (one with and one without a Pusher), as well as opens us up to writing tests that validate expected functionality such as:
"Test that Name and Runtime are required or error"
"Test that Tag defaults to DefaultTag (quay.io)"
"Test that a Builder respects the Push boolean configuration"
etc.
There was a problem hiding this comment.
Excellent points. I have moved it in to function.go and renamed it to faas.FunctionConfiguration(). I think it might benefit from adding a name parameter as well in order to update the function name. But to be honest, it's a little clumsy right now and I'm not sure if this is the best way to approach it all in the long run.
There was a problem hiding this comment.
We probably have several solid refactors ahead of us where we can make it less clumsy!
Based on review feedback, this function really should not be part of the CLI itself, but rather in the client library. In this case, I have moved it to function.go as part of the faas package. Currently it provides the ability to override the image tag, but probably should be modified to also allow for a change to the function name as well.
|
@matejvasek @lkingland your comments are incorporated, and I've removed the WIP status. PTAL and if it looks ok, let's go ahead and merge so that Naina can start experimenting with it. |
| f.Tag = tag | ||
| } | ||
| if f.Tag == "" { | ||
| f.Tag = fmt.Sprintf("quay.io/%s:latest", f.Name) |
There was a problem hiding this comment.
Possible future improvement: I think we could pick better default by looking into docker config file like
the CompleteRegistryList function does.
This commit comprises some fairly large changes in the codebase.
The 'create' command has been extracted into 'init', 'bulid' and
'deploy' commands. The 'create' command remains, but now delegates
most of its work to these other three. This also has resulted in
some rework of the various flags. One example is the removal of
the '--local' flag from create, since this can be achieved now by
just running 'faas init'.
There are still a number of changes needed for this to land.
from --help text and should be returned
The progress meter is a little wonky nowThere are no tests, and I'm pretty sure I've broken what tests already existed@lkingland @matejvasek I welcome any comments, suggestions, modifications, etc.
It's a start - a work in progress.