NewDefaultAuthLoader() or var Loader = &defaultAuthLoader{}, but not DefaultAuthLoader

@smarterclayton Do you know what this "mapping *meta.RESTMapping" is supposed for? Looks like it was a dead parameter even before I started. Was it just cruft of did someone forget to wire it up?

It's used by Openshift. This implementation does not need it.

On Nov 18, 2014, at 11:15 AM, David Eads notifications@github.com wrote:

In pkg/kubectl/cmd/cmd.go:

@@ -46,11 +49,11 @@ func NewFactory() *Factory {
return &Factory{
Mapper: latest.RESTMapper,
Typer: api.Scheme,

•   Client: func(cmd *cobra.Command, mapping *meta.RESTMapping) (kubectl.RESTClient, error) {
  

  @smarterclayton Do you know what this "mapping *meta.RESTMapping" is supposed for? Looks like it was a dead parameter even before I started. Was it just cruft of did someone forget to wire it up?

—
Reply to this email directly or view it on GitHub.

Do people want to put a secret as a flag argument. Seems like that risks exposure via proc or accidental pasting.
The other secrets are in files.

Do people want to put a secret as a flag argument.

Allowing the token as a command line parameter makes end-to-end testing more convenient. The recommended path would still be to use the _auth file to avoid having secrets in your command history.

On Nov 18, 2014, at 11:42 AM, Eric Tune notifications@github.com wrote:

In pkg/client/cmd/client_builder.go:

• "github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
• "github.com/GoogleCloudPlatform/kubernetes/pkg/client"
• "github.com/GoogleCloudPlatform/kubernetes/pkg/clientauth"
• "github.com/GoogleCloudPlatform/kubernetes/pkg/version"
  +)

+const (

• FlagApiServer = "server"
• FlagMatchApiVersion = "match-server-version"
• FlagApiVersion = "api-version"
• FlagAuthPath = "auth-path"
• FlagInsecure = "insecure-skip-tls-verify"
• FlagCertFile = "client-certificate"
• FlagKeyFile = "client-key"
• FlagCAFile = "certificate-authority"
• FlagBearerToken = "token"
  Do people want to put a secret as a flag argument. Seems like that risks exposure via proc or accidental pasting.
  The other secrets are in files.

Is there no scenario in which you would need/want to use the CLI flag? Agree it's not best practice, but it seems occasionally useful.

Don't remove cmd from these methods - a consumer may need to look at flags in order to properly retrieve the values.

Shouldn't this be f.Client(cmd,mapping)? I don't think you need to look at the builder here.

Shouldn't this be f.Client(cmd,mapping)? I don't think you need to look at the builder here.

The types are not directly compatible. The clientFunc needs to return a client.RESTClient, but the Factory.Client() returns a kubectl.RESTClient. All client.RESTClients are kubectl.RESTClients, but not the other way around.

@smarterclayton I made the changes required to directly call the Client() method, but I'm not sure whether you want the pkg/config depending on pkg/kubectl. I've left it as a separate commit so you can decide.

"cmd" doesn't convey to me what the docs describe. Any other names?

"cmd" doesn't convey to me what the docs describe. Any other names?

clientcmd? That would make the qualified package pkg/client/clientcmd. http does this with net/http/httputil, but I had noted the kubernetes had pkg/kubectl/cmd.

On Nov 20, 2014, at 6:46 AM, David Eads notifications@github.com wrote:

In pkg/client/cmd/doc.go:

+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+

• http://www.apache.org/licenses/LICENSE-2.0

+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+/
+
+/
+Package cmd provides one stop shopping for a command line executable to bind the correct flags,
"cmd" doesn't convey to me what the docs describe. Any other names?

clientcmd? That would make the qualified package pkg/client/clientcmd. http does this with net/http/httputil, but I had noted the kubernetes had pkg/kubectl/cmd.

I wouldn't take that as a constraint. clientcmd sounds fine.

—
Reply to this email directly or view it on GitHub.

No function BuildClient defined in this PR that I can see.

Fixed to .Client()

I don't think it is necessary to introduce a Builder type.
You can do this in an interactive client:

apiClient, err := clientcmd.NewFromPersistentFlags(cmds.PersistentFlags())
if os.IsNotExist(err) {
    apiClient, err := clientcmd.NewFromPrompt(cmds.PersistentFlags())
}
if err {
  fmt.Println("Could not make a client", err)
}

For the case of a non-interactive client things simplify to:

apiClient, err := clientcmd.New()
if err {
  fmt.Println("Could not make a client", err)
}

This makes non-interactive client code more concise, and I expect there will be more non-interactive clients than interactive ones.
Even for interactive clients, I find it preferable, in terms of readability, to get forego introducing a new type and gain a few more lines of error handling (which will be familiar to any go programmer.)

One of the issues today is keeping the flag names consistent across multiple different commands. That means that a BindFlags method is still required (or at least a call to create the flags). So we're looking having two calls no matter what we do.

By having an object holding that state information (the builder), we avoid having two discrete calls
clientcmd.BindFlags(cmd.PersistentFlags())
clientcmd.Client(cmd.PersistentFlags))
that rely upon having two different methods being called with matching arguments. This is especially useful when you look at the common kubernetes usage that has methods trying to build clients relatively removed from the the point where the flags are bound.

Also we may be instantiating multiple clients - the kubectl use needs that (so you map restclients across multiple component plugins). Builder just happened to be a convenient holder for that. Having a simpler path for single client types isn't bad, and seems like that could be a simple static call.

On Nov 24, 2014, at 3:31 PM, David Eads notifications@github.com wrote:

In pkg/client/clientcmd/doc.go:

+You may obtain a copy of the License at
+

• http://www.apache.org/licenses/LICENSE-2.0

+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+/
+
+/
+Package cmd provides one stop shopping for a command line executable to bind the correct flags,
+build the client config, and create a working client. The code for usage looks like this:
+

• clientBuilder := clientcmd.NewBuilder(clientcmd.NewDefaultAuthLoader())
  One of the issues today is keeping the flag names consistent across multiple different commands. That means that a BindFlags method is still required (or at least a call to create the flags). So we're looking having two calls no matter what we do.

By having an object holding that state information (the builder), we avoid having two discrete calls
clientcmd.BindFlags(cmd.PersistentFlags())
clientcmd.Client(cmd.PersistentFlags))
that rely upon having two different methods being called with matching arguments. This is especially useful when you look at the common kubernetes usage that has methods trying to build clients relatively removed from the the point where the flags are bound.

—
Reply to this email directly or view it on GitHub.

I'm skeptical that clients other than kubectl (non-interactive ones) will want to use persistent flags. And, in the case of kubectl, the two lines in question are right next to each other.

Call site for binding seems like it could be in many places (usually in init, while client is going to be in main), and persistent flags and other flags are both FlagSet.

Did you just want a more realistic example?

Builder feels to me like using a pattern for the sake of using patterns.

Hrm. Usually you create an object and then bind its params with *VarP, and then use the object you bound to get a client. So it feels patterny, but that because it's a pattern :)

You have to bind before you can create a client, and you can't read the values you bound until after flags are parsed. The example doesn't show that and it should. So it's always a two step process.

On Nov 25, 2014, at 7:19 PM, Eric Tune notifications@github.com wrote:

In pkg/client/clientcmd/doc.go:

+You may obtain a copy of the License at
+

• http://www.apache.org/licenses/LICENSE-2.0

+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+/
+
+/
+Package cmd provides one stop shopping for a command line executable to bind the correct flags,
+build the client config, and create a working client. The code for usage looks like this:
+

• clientBuilder := clientcmd.NewBuilder(clientcmd.NewDefaultAuthLoader())
  Builder feels to me like using a pattern for the sake of using patterns.

—
Reply to this email directly or view it on GitHub.

BuildConfig is not defined anywhere - Config?

Typo from an early refactor

On Nov 26, 2014, at 4:56 PM, Tim Hockin notifications@github.com wrote:

In pkg/client/clientcmd/client_builder.go:

• "github.com/spf13/pflag"

• "github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
• "github.com/GoogleCloudPlatform/kubernetes/pkg/client"
• "github.com/GoogleCloudPlatform/kubernetes/pkg/clientauth"
• "github.com/GoogleCloudPlatform/kubernetes/pkg/version"
  +)

+// Builder are used to bind and interpret command line flags to make it easy to get an api server client
+type Builder interface {

• // BindFlags must bind and keep track of all the flags required to build a client config object
• BindFlags(flags *pflag.FlagSet)
• // Config uses the values of the bound flags and builds a complete client config
• Config() (*client.Config, error)
• // Client calls BuildConfig under the covers and uses that config to return a client
  BuildConfig is not defined anywhere - Config?

—
Reply to this email directly or view it on GitHub.