-
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
CLI harmonization and api simplification #79
CLI harmonization and api simplification #79
Conversation
Note: if we updated cobra to some newer version (via fix commit hash, since there is no official release) we could get rid off: |
We could write completion for the registry flag. The docker file config contains both registry URI and username. However there is one more thing to consider: push destination doesn't have to match patter like registry.url/USERNAME. For instance OpenShift internal registry doesn't use usernames, but namespaces, so URI to push looks like my-cluster.com/default. |
👍 for --image instead of --tag |
I haven't gone through the code yet, but I would like to respond to some of the text in your (excellent) PR description before I forgot about it.
Now... there's a lot of code to look at. 👍 |
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.
Whew! Lots of good changes in here.
project, err := k8s.ToSubdomain(name) | ||
func (d *Deployer) Deploy(f faas.Function) (err error) { | ||
// k8s does not support service names with dots. so encode it such that | ||
// www.my-domain,com -> www-my--domain-com |
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.
Personally, I don't like the double dashes where my-domain
becomes my--domain
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 most certainly do not either. Unfortunately that's a restriction placed on us by k8s, which requires the positively draconian RFC 1035 label:
labels must follow the rules for ARPANET host names. They must
start with a letter, end with a letter or digit, and have as interior
characters only letters, digits, and hyphen.
So restrictive, the only option I could think of was to overload hyphen with itself or make it ambiguous :/
53f86fc
to
2a63c24
Compare
- updated tests to new api throughout - expanded tests where appropriate - lint issues - minor code review comments addressed
5b591bc
to
00ac880
Compare
This sounds like a good idea to me. I have tried to use the term "Registry + namespace" to refer to the first two tokens of the image's name. So that should fit with either the OpenShift or Docker nomenclature. I've created #81 and #82 to encapsulate this suggestion and the need to expand and update the shell integrations. |
- Replaces globally-scoped formatter function with methods - Defines enumerated Format types - Renames the 'output' flag 'format' due to confusion with command file descriptors - FunctionDescription now Function - Global verbose flag replaced with config struct based value throughout
Thanks so much for the input @lance and @matejvasek . I hope all conversations were either resolved to your satisfaction, or were referred to a newly created issue to track further conversation. I am closing this PR now in order to get it into the hands of Naina for testing and Roland for evaluation/potential kn integration. Please feel free to continue to comment on anything herein, especially the more recent commits addressing issues, if you have questions, suggestions or concerns. Thanks again |
Hi folks,
This PR makes an attempt to leveling up our CLI game. Improving flag consistency and general usability, fixing several bugs, while also simplifying and clarifying the code as a whole. I sincerely apologize for the large diff, and recommend perhaps reading the source files top-to-bottom instead, as that will very likely make more sense than trying to follow the line-by-line.
This is a WIP, and I will request review when the following are complete:
Closes #73 #76 #9
tl;dr
Assuming the environmental configuration:
The following are examples of valid commands (a subset which are relevant to this PR):
Creates a new project named
www.example.com
in the current directory, irrespective of path.Creates with an explicit runtime.
Results in the derived image (formerly tag)
docker.io/alice/www.example.com:latest
(note the Docker.io default registry).Results in the partially-derived image tag
quay.io/bob/www.example.com:latest
(note the default registry has been overridden as quay.io).Results in the explicitly provided image tag in full (ignores FAAS_REPOSITORY).
Deploys using the value of
--image
specified during previous build if provided.Builds and deploys the function to https://www.example.com/
Ignores the current kuberenetes config active namespace setting (
faas
if they followed the docs) and explicitly updates the function to be deployed in thefaas
namespace.Combined Example
Assuming DNS is configued to route the domain to the cluster, and the current environmental contains:
Creates a new Function, verifiable with
curl https://www.boson-project.org
.--
Implementation notes.
Why --image instead of --tag
The --tag flag was renamed to --image in keeping with the docker nomenclature and documentaiton, wherein the 'Image' is the full image name (for example
docker.io/alice/my.funciton.name:latest
), consisting of Registry (docker.io), User at registry (alice), and container (my.funciton.name), and tag (:latest). I was at first confused that the--tag
flag meant, literally only the tag (:latest
), so I presume others might be confused as well. I hope that using --image alleviates this ambiguity, but I can be convinced otherwise.Where's --build?
The --build flag was omitted from the
deploy
command, because the implementation complexity grew to encapsulate all that from thebuild
subcommand, with the simplest method of building and deploying being tofaas build && faas deploy
.Where's embedded.Initializer?
The Initializer was renamed a TemplateWriter to more accurately convey its more curtailed purpose now that Appsody has been removed. Furthermore, as there is likely no chance there will be future alternate implementaitons of a TemplateWriter other than the current "embedded" version, its functionality was merged into the core Client logic. Its only optional configuration parameter, the path to extensible templates, was thus moved to be an optional constructor option on Client. This simplifies the instantiation of a Client (no need to create an
embedded.Initializer
anymore), and removes the name collision of the Initialize method (on Client, it initialized a new funciton project and all ancillary tasks, on the old Intializer, it merely wrote templates to disk).One Config per Command
Each of the commands was given its own config object, with interactive user prompts plumbed in using a config.Prompt() method. This allows for all configuration parameters to be expressed on the config in question, even if repeated (something not possible with struct embeddeing, in which case attributes had to be fully disjunct sets).
Trigger vs Template
The command parameter
trigger
, which I noticed had been partially implemented, and appeared to be in place oftemplate
has now been adopted throughout. I am fine with eithertrigger
ortemplate
, but this is worth additional discussion, because future values may imply more than just the triggering event, but details about the containing context. I.e.nodejs-express-webpack
or some such.Default to HTTP instead of CloudEvents
The default trigger has been set to
http
because new users should be able tocurl https://mynewfunction
without needing to first construct a cryptic cloud event. This use case became evident to me as the base use case when writing the first draft of the users guide, in which it is clear that getting the user to a simple HTTP request/response using standard HTTP primitives is an important first step before moving on to cloud events (a step step up in complexity which may not even be necessary).Argument and Attribute Alphanumeric Ordering
Flag, parameters and arguments were alphabatized. This practice is not always appropriate, but since in this case there is no clear precidence of the attributes, any non-alphanumeric sorting implies a hierarchy or sorting logic which is not immediately scritable, leading to the uncomfortable nagging feeling something important is not being understood. Alphanumeric sorting I find nips this at the bud.
Where's --push
Push is omitted because it is an implementation detail of the deploy step, and plumbing through the Push method exposed by the internal Pusher causes an overlap of functionality in the Client's public API that has to be explained away with lots of comments. This has a bit of code-smell, so perhaps we can reply on the fact that we expect the registry, image name, and pushing to be completely an implementaiton detail of the Deployment process, with the need to push a specific image being a debugging feature, and thus can be accomplished with
docker push
. If this is an important thing to keep in our API, I am open to reasons, it just seems at this point the exposure of unnecessary internals, as is made evident when I actually tried to expose it elegantly without much success.the --push flag on the build command hadn't actually been attached, so that one was an easier decision to remove.
arg[0] is Name, added --runtime [language]
Per suggestions from users early on, the command argument to
create
andinit
is now the name of the function, rather than the runtime. Runtime is now a flag--runtime quarkus
. This certainly makes more sense, and I consider it an oversight on my part to have confused them in the first place.This deaults to runtime nodejs, overridden by config environment variable FAAS_RUNTIME, or can be explicitly provided:
Where's NewFunctionConfiguration?
Now that the parameters are logically defaulted with consistent hierarchy, the new constructor
NewFunctionConfiguration
could be rolled in to the original constructorNewFunction
.Public API takes Function root path, internal interfaces take a faas.Function proper
With the function configuration now properly defaulted, the interfaces for the interstitial steps were updated to all take a single argument, the path at which to find the code necessary to operate on. This could just has easily have been an instance of the Function object (creating a new instance is as easy as
new Function(path)
, but is still an unnecessary extra step a user of the client library must take, with a string path being about as easy as it gets. We may want to revisit this in the future if we want the client library to be something a bit more flexible at the expense of API simplicity.Single-character Function Receivers
Places where the function receiver's name was a full name, such as on the knative.Deployer, were renamed to the conventional single-character per Go best practices (https://github.com/golang/go/wiki/CodeReviewComments#receiver-names)
Where's --expose?
The --expose flag was omitted from the
deploy
command for now because it was not currently implemented, and exposure is the default when the cluster is configured correctly. While not yet tested, a function whose name is not in the list of configured domains, or is explicitly with the suffix.cluster.local
should be created without a public route.Make tweaks
Make clean now removes the default binary as well as the cross-compiled versions.
Reconciling Duplicate CLI Output Format Implementations
There were two different implementations of function output formatting. An attempt was made to reduce this to a single solution by adding 'plain' to one and adding default serilizations to both. At the time of this writing there's more work to be done there, but I am puting on that in favor of bigger fish.
Unused Registry Flag Completion function
Since none of the flags are now the bare registry, (the --repository flag is registry plus user at the registry), the completion function is no longer used. Perhaps we should split this back into two environment variables, a registry and registryuser, such that we can easily default. Definitely an item for discussion, but for now one can pass
--repository dockerusername
or--repository docker.io/dockerusernam
to the same effect.Now "Function" througout
Removed all instances of "service function" and lower-case "function" when referring to a FaaS Function, opting to use the proper noun "Function", short for a Boson Project Function as a Service. lower-case "function", usually in comments, refers as per Go language nomenclature to a method with a receiver.
Version subcommand
Fixed bug where version was being updated by first run (when used by Cobra) altering output in final call (only affected from-source builds).
Namespace Delegation
The Namespace parameter is now treated as an override, with the default being to allow an unset value which indicates the currently active namespace from ~/.kube/config (usualy) is used.
Default to docker registry
When the value of FAAS_REPOSITORY is set to a single token, such as
alice
, the repository prefixdocker.io/alice
is assumed. This can be overridden by either providing --repository explicitly, or a full --image.Acceptance Tests
To verify, I have run the following tests as a spot-check on usability. Many of these could, and should, be automated with a robust test suite in the
main
package, but the important test suite is that which validats the Client API, which takes precidence in this update due to time constraints.version
make
yields aversion
ofv0.0.0
make
yields aversion --verbose
ofv0.0.0-GITHASH-DATESTAMP
--version
from CLI library is connected.go build ./cmd/faas
directly yieldstip
go build ./cmd/faas
directly yieldstip
with or without--verbose
init
init my.example.com
yields tempalte files and a correctly populated .faas.configinit
from within a conformant direcotory yields template files and a populated .faas.configinit my.example.com --path ./test
runs as though run from within./test
init
in a non-empty directory fails with errorbuild
build
using .faas.config, image derived from FAAS_REPOSITORY (defauting to docker.io if not prepended) and updates .faas.config with iamgebuild --image quay.io/boson/my.example.com:v0.0.1
overrides derived image; function config is updated.build --path ./test
runs as though run from within./test
deploy
deploy
defaults to the namespace defined in ~/.kube/configdeploy
creates a publicly-accessible, TLS-secured routeTODO: deploy to alternate namespace (can test after
delete
)TODO: deploy using explicit --path (can test after
delete
)delete
delete
a function from path-derived namedelete
a function from an initialized function uses the name defined in .faas.configdelete --path ./test
runs as though run from within./test
list
list
when there are no deployed functions correctly returns nothinglist
when there are functions lists them.TODO: check the different --output formats
create
create
from a compliant domain path completely creates a running Function, routed and TLS secured.describe
describe
using explicit namedescribe
using name from current working directory's deploymentdescribe
using the--path
parameterupdate
update
using name from current working directoryrun
TODO: run is currently a WIP
completion
TODO: regenerate/reintegrate shell completion now that commands have shifted slightly