Skip to content
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.

Sign up for GitHub

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

BUGFIX: re-enable install test and fix flags which were not parsed correctly #14

Merged
merged 1 commit into from
Jan 26, 2024
Merged

BUGFIX: re-enable install test and fix flags which were not parsed correctly #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions pkg/factory/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,12 @@ import (
cmclient "github.com/cert-manager/cert-manager/pkg/client/clientset/versioned"
)

var (
kubeConfigFlags = genericclioptions.NewConfigFlags(true)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made these global variables into local variables, so the test can parse flags multiple times without issues.

factory = util.NewFactory(kubeConfigFlags)
)

// Factory provides a set of clients and configurations to authenticate and
// access a target Kubernetes cluster. Factory will ensure that its fields are
// populated and valid during command execution.
type Factory struct {
factory util.Factory

// Namespace is the namespace that the user has requested with the
// "--namespace" / "-n" flag. Defaults to "default" if the flag was not
// provided.
Expand All @@ -66,23 +63,32 @@ type Factory struct {

// New returns a new Factory. The supplied command will have flags registered
// for interacting with the Kubernetes access options. Factory will be
// populated when the command is executed using the cobra PreRun. If a PreRun
// populated when the command is executed using the cobra PreRunE. If a PreRunE
// is already defined, it will be executed _after_ Factory has been populated,
// making it available.
func New(ctx context.Context, cmd *cobra.Command) *Factory {
f := new(Factory)

kubeConfigFlags := genericclioptions.NewConfigFlags(true)
f.factory = util.NewFactory(kubeConfigFlags)

kubeConfigFlags.AddFlags(cmd.Flags())
cmd.RegisterFlagCompletionFunc("namespace", validArgsListNamespaces(ctx, f))

// Setup a PreRun to populate the Factory. Catch the existing PreRun command
// Setup a PreRunE to populate the Factory. Catch the existing PreRunE command
// if one was defined, and execute it second.
existingPreRun := cmd.PreRun
cmd.PreRun = func(cmd *cobra.Command, args []string) {
util.CheckErr(f.complete())
if existingPreRun != nil {
existingPreRun(cmd, args)
// WARNING: Do not set PreRun on the command as cobra will not execute PreRun when
// PreRunE is set.
Comment on lines +80 to +81
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only figured this out after reading their source-code.

existingPreRunE := cmd.PreRunE
cmd.PreRunE = func(cmd *cobra.Command, args []string) error {
if err := f.complete(); err != nil {
return err
}

if existingPreRunE != nil {
return existingPreRunE(cmd, args)
}
return nil
}

return f
Expand All @@ -93,12 +99,12 @@ func New(ctx context.Context, cmd *cobra.Command) *Factory {
func (f *Factory) complete() error {
var err error

f.Namespace, f.EnforceNamespace, err = factory.ToRawKubeConfigLoader().Namespace()
f.Namespace, f.EnforceNamespace, err = f.factory.ToRawKubeConfigLoader().Namespace()
if err != nil {
return err
}

f.RESTConfig, err = factory.ToRESTConfig()
f.RESTConfig, err = f.factory.ToRESTConfig()
if err != nil {
return err
}
Expand All @@ -113,7 +119,7 @@ func (f *Factory) complete() error {
return err
}

f.RESTClientGetter = factory
f.RESTClientGetter = f.factory

return nil
}
12 changes: 6 additions & 6 deletions pkg/install/helm/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ func NewNormalisedEnvSettings() *NormalisedEnvSettings {
ActionConfiguration: &action.Configuration{},
}
}

func (n *NormalisedEnvSettings) Namespace() string {
return n.Factory.Namespace
}
Expand Down Expand Up @@ -97,23 +96,24 @@ func (n *NormalisedEnvSettings) setupEnvSettings(ctx context.Context, cmd *cobra
{
// Add a PreRun hook to set the debug value to true if the log level is
// >= 3.
existingPreRun := cmd.PreRun
cmd.PreRun = func(cmd *cobra.Command, args []string) {
existingPreRunE := cmd.PreRunE
cmd.PreRunE = func(cmd *cobra.Command, args []string) error {
if n.logger.V(debugLogLevel).Enabled() {
n.EnvSettings.Debug = true
}

if existingPreRun != nil {
existingPreRun(cmd, args)
if existingPreRunE != nil {
return existingPreRunE(cmd, args)
}
return nil
}
}
}

func (n *NormalisedEnvSettings) InitActionConfiguration() error {
return n.ActionConfiguration.Init(
n.Factory.RESTClientGetter,
n.EnvSettings.Namespace(),
n.Factory.Namespace,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The n.Factory.Namespace seems like a better source-of-truth to get the namespace.

os.Getenv("HELM_DRIVER"),
func(format string, v ...interface{}) {
n.logger.Info(fmt.Sprintf(format, v...))
Expand Down
5 changes: 2 additions & 3 deletions test/integration/ctl_install.go → test/integration/ctl_install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (

"github.com/cert-manager/cmctl/v2/cmd"
"github.com/cert-manager/cmctl/v2/test/integration/install_framework"
"github.com/cert-manager/cmctl/v2/test/integration/internal/util"
logsapi "k8s.io/component-base/logs/api/v1"
)

func TestCtlInstall(t *testing.T) {
Expand Down Expand Up @@ -99,12 +99,11 @@ func executeCommandAndCheckOutput(
stdin := bytes.NewBufferString("")
stdout := bytes.NewBufferString("")

chartPath := util.GetTestPath("deploy", "charts", "cert-manager", "cert-manager.tgz")
logsapi.ResetForTest(nil)
cmd := cmd.NewCertManagerCtlCommand(ctx, stdin, stdout, stdout)
cmd.SetArgs(append([]string{
fmt.Sprintf("--kubeconfig=%s", kubeConfig),
"--wait=false",
fmt.Sprintf("--chart-name=%s", chartPath),
"x",
"install",
}, inputArgs...))
Expand Down
7 changes: 5 additions & 2 deletions test/integration/install_framework/framework.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@ import (
"os"
"testing"

"github.com/cert-manager/cert-manager/test/apiserver"
"github.com/go-logr/logr/testr"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/envtest"

"github.com/cert-manager/cert-manager/test/apiserver"
"sigs.k8s.io/controller-runtime/pkg/log"
)

type TestInstallApiServer struct {
Expand All @@ -39,6 +40,8 @@ type TestInstallApiServer struct {
type CleanupFunction func()

func NewTestInstallApiServer(t *testing.T) (*TestInstallApiServer, CleanupFunction) {
log.SetLogger(testr.New(t))

env, stopFn := apiserver.RunBareControlPlane(t)

testUser, err := env.ControlPlane.AddUser(
Expand Down