Hi @perotinus. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

This change is 

@madhusudancs Ping? Hopefully this shouldn't be too hard to review.

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@madhusudancs Can you review this when you get a chance?

One question: since kubefed is now beta, do we need to keep supporting the --secret-name flag?

@perotinus I think deprecation policy's rule #5b applies to our case here since kubefed is an admin-facing CLI element and it's in beta. So we need to mark --secret-name as deprecated in v1.7 and remove it in v1.8. There are some examples for marking flags as deprecated here - https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-apiserver/app/options/options.go and in various other commands.

@k8s-bot ok to test

@perotinus mostly minor nits. Please ping me once they are addressed.

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

federation/pkg/kubefed/join.go, line 80 at r1 (raw file):

func (o *joinFederationOptions) Bind(flags *pflag.FlagSet) {
	flags.StringVar(&o.clusterContext, "cluster-context", "", "Name of the cluster's context in the local kubeconfig. Defaults to cluster name if unspecified.")
	flags.StringVar(&o.secretName, "secret-name", "", "Name of the secret where the cluster's credentials will be stored in the host cluster. This name should be a valid RFC 1035 label. Defaults to cluster name if unspecified.")

Just leaving a note to mark this as deprecated.

federation/pkg/kubefed/join.go, line 125 at r1 (raw file):

	if j.options.secretName == "" {
		j.options.secretName = j.commonOptions.Name
	}

Note to bring it back for the deprecated flag.

federation/pkg/kubefed/join.go, line 137 at r1 (raw file):

		return err
	}
	secretName := v1.SimpleNameGenerator.GenerateName(j.commonOptions.Name)

We generally use a - to separate the prefix from the generated part. It is arguably easier to read the generated name that way. I think it is preferable to do it here too. GenerateName() doesn't do that for us unfortunately, so we should do it ourselves.

federation/pkg/kubefed/join_test.go, line 201 at r1 (raw file):

				}
				// The secret name is generated, so replace it with the test secret name
				got.Spec.SecretRef.Name = testSecretName

Feel free to ignore:

Too implementation dependent, but this is a unit test. So instead check if the <prefix>- matches the cluster name?

federation/pkg/kubefed/join_test.go, line 306 at r1 (raw file):

				}
				// The secret name is generated, so replace it with the test secret name
				got.Name = testSecretName

Same here.

Comments from Reviewable

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

federation/pkg/kubefed/join.go, line 80 at r1 (raw file):

Done.

federation/pkg/kubefed/join.go, line 125 at r1 (raw file):

Done.

federation/pkg/kubefed/join_test.go, line 201 at r1 (raw file):

I've done something similar: I think the key thing that we care about in re the generated secret name is that it's not zero length, so I'm testing that.

Comments from Reviewable

@perotinus I have one open question.

Reviewed 2 of 3 files at r2.
Review status: 3 of 4 files reviewed at latest revision, 3 unresolved discussions.

federation/pkg/kubefed/join.go, line 81 at r2 (raw file):

func (o *joinFederationOptions) Bind(flags *pflag.FlagSet) {
	flags.StringVar(&o.clusterContext, "cluster-context", "", "Name of the cluster's context in the local kubeconfig. Defaults to cluster name if unspecified.")
	flags.StringVar(&o.secretName, "secret-name", "", "Name of the secret where the cluster's credentials will be stored in the host cluster. This name should be a valid RFC 1035 label. Defaults to cluster name if unspecified.")

Can we also say "DEPRECATED" in the string somewhere here? Or is that auto-generated by the flags library?

Comments from Reviewable

Review status: 3 of 4 files reviewed at latest revision, 3 unresolved discussions.

federation/pkg/kubefed/join.go, line 81 at r2 (raw file):

The flags library deals with that:

Flag --secret-name has been deprecated, kubefed now generates a secret name, and this flag will be removed in a future release.

Comments from Reviewable

/approve

Reviewed 1 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

federation/pkg/kubefed/join.go, line 81 at r2 (raw file):

Awesome!

Comments from Reviewable

@k8s-bot pull-kubernetes-federation-e2e-gce test this

Review status: all files reviewed at latest revision, 4 unresolved discussions.

federation/cluster/federation-up.sh, line 110 at r2 (raw file):

        --federation-system-namespace=${FEDERATION_NAMESPACE} \
        --host-cluster-context="${HOST_CLUSTER_CONTEXT}" \
        --context="${FEDERATION_KUBE_CONTEXT}" \

@perotinus Federation presubmit failed - https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/42513/pull-kubernetes-federation-e2e-gce/2697/?log#log

Sorry, I wasn't paying attention. But I am so happy that we have presubmits now! Trailing \ should be removed here.

Comments from Reviewable

Review status: all files reviewed at latest revision, 4 unresolved discussions.

federation/cluster/federation-up.sh, line 110 at r2 (raw file):

D'oh! Fixed.

Comments from Reviewable

Review status: all files reviewed at latest revision, 4 unresolved discussions.

federation/pkg/kubefed/join.go, line 137 at r1 (raw file):

Done.

federation/pkg/kubefed/join_test.go, line 306 at r1 (raw file):

Done.

Comments from Reviewable

@perotinus: The following test(s) failed:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

@madhusudancs Can you rerun the Federation e2e tests when you get a chance?

@k8s-bot pull-kubernetes-federation-e2e-gce test this

/lgtm

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: madhusudancs, perotinus

Automatic merge from submit-queue (batch tested with PRs 45100, 45152, 42513, 44796, 45222)