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

Jump to bottom

fix: test and namespace isolation #2187

Merged
merged 5 commits into from
May 17, 2024
Merged

fix: test and namespace isolation #2187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f383ac7
fix: namespace logic cleanup and test isolation
lkingland Apr 5, 2024
d6a1467
fix spelling errors
lkingland Apr 24, 2024
e677216
fix: pipelines remover always returned (empty) error
lkingland May 8, 2024
1a13f62
update tekton namespace handling
lkingland May 8, 2024
98dc85b
fix: remove deprecated GitLab config: max_concurrency
lkingland May 17, 2024
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
3 changes: 2 additions & 1 deletion cmd/build_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

fn "knative.dev/func/pkg/functions"
"knative.dev/func/pkg/mock"
. "knative.dev/func/pkg/testing"
)

// TestBuild_BuilderPersists ensures that the builder chosen is read from
Expand Down Expand Up @@ -100,7 +101,7 @@ func TestBuild_Authentication(t *testing.T) {
// - Push not triggered after an unsuccessful build
// - Push can be disabled
func TestBuild_Push(t *testing.T) {
root := fromTempDirectory(t)
root := FromTempDirectory(t)

f := fn.Function{
Root: root,
Expand Down
19 changes: 6 additions & 13 deletions cmd/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,6 @@ import (
// These are the minimum settings necessary to create the default client
// instance which has most subsystems initialized.
type ClientConfig struct {
// Namespace in the remote cluster to use for any client commands which
// touch the remote. Optional. Empty namespace indicates the namespace
// currently configured in the client's connection should be used.
Namespace string

// Verbose logging. By default, logging output is kept to the bare minimum.
// Use this flag to configure verbose logging throughout.
Verbose bool
Expand Down Expand Up @@ -62,16 +57,16 @@ func NewClient(cfg ClientConfig, options ...fn.Option) (*fn.Client, func()) {
var (
t = newTransport(cfg.InsecureSkipVerify) // may provide a custom impl which proxies
c = newCredentialsProvider(config.Dir(), t) // for accessing registries
d = newKnativeDeployer(cfg.Namespace, cfg.Verbose)
pp = newTektonPipelinesProvider(cfg.Namespace, c, cfg.Verbose)
d = newKnativeDeployer(cfg.Verbose)
pp = newTektonPipelinesProvider(c, cfg.Verbose)
o = []fn.Option{ // standard (shared) options for all commands
fn.WithVerbose(cfg.Verbose),
fn.WithTransport(t),
fn.WithRepositoriesPath(config.RepositoriesPath()),
fn.WithBuilder(buildpacks.NewBuilder(buildpacks.WithVerbose(cfg.Verbose))),
fn.WithRemover(knative.NewRemover(cfg.Verbose)),
fn.WithDescriber(knative.NewDescriber(cfg.Namespace, cfg.Verbose)),
fn.WithLister(knative.NewLister(cfg.Namespace, cfg.Verbose)),
fn.WithDescriber(knative.NewDescriber(cfg.Verbose)),
fn.WithLister(knative.NewLister(cfg.Verbose)),
fn.WithDeployer(d),
fn.WithPipelinesProvider(pp),
fn.WithPusher(docker.NewPusher(
Expand Down Expand Up @@ -117,9 +112,8 @@ func newCredentialsProvider(configPath string, t http.RoundTripper) docker.Crede
return creds.NewCredentialsProvider(configPath, options...)
}

func newTektonPipelinesProvider(namespace string, creds docker.CredentialsProvider, verbose bool) *tekton.PipelinesProvider {
func newTektonPipelinesProvider(creds docker.CredentialsProvider, verbose bool) *tekton.PipelinesProvider {
options := []tekton.Opt{
tekton.WithNamespace(namespace),
tekton.WithCredentialsProvider(creds),
tekton.WithVerbose(verbose),
tekton.WithPipelineDecorator(deployDecorator{}),
Expand All @@ -128,9 +122,8 @@ func newTektonPipelinesProvider(namespace string, creds docker.CredentialsProvid
return tekton.NewPipelinesProvider(options...)
}

func newKnativeDeployer(namespace string, verbose bool) fn.Deployer {
func newKnativeDeployer(verbose bool) fn.Deployer {
options := []knative.DeployerOpt{
knative.WithDeployerNamespace(namespace),
knative.WithDeployerVerbose(verbose),
knative.WithDeployerDecorator(deployDecorator{}),
}
Expand Down
26 changes: 14 additions & 12 deletions cmd/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ import (
"knative.dev/func/pkg/mock"
)

const namespace = "func"

// Test_NewTestClient ensures that the convenience method for
// constructing a mocked client for testing properly considers options:
// options provided to the factory constructor are considered exaustive,
Expand All @@ -24,30 +22,34 @@ func Test_NewTestClient(t *testing.T) {

// Factory constructor options which should be used when invoking later
clientFn := NewTestClient(fn.WithRemover(remover))

// Factory should ignore options provided when invoking
client, _ := clientFn(ClientConfig{}, fn.WithDescriber(describer))

// Trigger an invocation of the mocks
err := client.Remove(context.Background(), fn.Function{Name: "test", Deploy: fn.DeploySpec{Namespace: namespace}}, true)
if err != nil {
t.Fatal(err)
}
f, err := fn.NewFunction("")
// Trigger an invocation of the mocks by running the associated client
// methods which depend on them
err := client.Remove(context.Background(), "myfunc", "myns", fn.Function{}, true)
if err != nil {
t.Fatal(err)
}
_, err = client.Describe(context.Background(), "test", f)
_, err = client.Describe(context.Background(), "myfunc", "myns", fn.Function{})
if err != nil {
t.Fatal(err)
}

// Ensure the first set of options, held on the factory, were used
// Ensure the first set of options, held on the factory (the mock remover)
// is invoked.
if !remover.RemoveInvoked {
t.Fatalf("factory (outer) options not carried through to final client instance")
}
// Ensure the second set of options, provided when constructing the
// client using the factory, were ignored
// Ensure the second set of options, provided when constructing the client
// using the factory, are ignored.
if describer.DescribeInvoked {
t.Fatalf("test client factory should ignore options when invoked.")
}

// This ensures that the NewTestClient function, when provided a set
// of optional implementations (mocks) will override any which are set
// by commands, allowing tests to "force" a command to use the mocked
// implementations.
}
4 changes: 2 additions & 2 deletions cmd/completion_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ import (
)

func CompleteFunctionList(cmd *cobra.Command, args []string, toComplete string) (strings []string, directive cobra.ShellCompDirective) {
lister := knative.NewLister("", false)
lister := knative.NewLister(false)

list, err := lister.List(cmd.Context())
list, err := lister.List(cmd.Context(), "")
if err != nil {
directive = cobra.ShellCompDirectiveError
return
Expand Down
22 changes: 2 additions & 20 deletions cmd/config_git_remove.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func NewConfigGitRemoveCmd(newClient ClientFactory) *cobra.Command {
such as local generated Pipelines resources and any resources generated on the cluster.
`,
SuggestFor: []string{"rem", "rmeove", "del", "dle"},
PreRunE: bindEnv("path", "namespace", "delete-local", "delete-cluster"),
PreRunE: bindEnv("path", "delete-local", "delete-cluster"),
Copy link
Member

Choose a reason for hiding this comment

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

the ns comes from?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just cleanup. That command does not use namespace. This command removes git settings from the function's metadata. This is not func delete which actually removes the running service (and requires namespace)

RunE: func(cmd *cobra.Command, args []string) (err error) {
return runConfigGitRemoveCmd(cmd, newClient)
},
Expand All @@ -37,20 +37,6 @@ func NewConfigGitRemoveCmd(newClient ClientFactory) *cobra.Command {
fmt.Fprintf(cmd.OutOrStdout(), "error loading config at '%v'. %v\n", config.File(), err)
}

// Function Context
f, _ := fn.NewFunction(effectivePath())
if f.Initialized() {
cfg = cfg.Apply(f)
}

// Flags
//
// Globally-Configurable Flags:
// Options whose value may be defined globally may also exist on the
// contextually relevant function; but sets are flattened via cfg.Apply(f)
cmd.Flags().StringP("namespace", "n", cfg.Namespace,
"Deploy into a specific namespace. Will use function's current namespace by default if already deployed, and the currently active namespace if it can be determined. ($FUNC_NAMESPACE)")

// Resources generated related Flags:
cmd.Flags().Bool("delete-local", false, "Delete local resources (pipeline templates).")
cmd.Flags().Bool("delete-cluster", false, "Delete cluster resources (credentials and config on the cluster).")
Expand All @@ -69,8 +55,6 @@ type configGitRemoveConfig struct {
// working directory of the process.
Path string

Namespace string

// informs whether any specific flag for deleting only a subset of resources has been set
flagSet bool

Expand All @@ -93,8 +77,6 @@ func newConfigGitRemoveConfig(_ *cobra.Command) (c configGitRemoveConfig) {
}

c = configGitRemoveConfig{
Namespace: viper.GetString("namespace"),

flagSet: flagSet,

metadata: pipelines.PacMetadata{
Expand Down Expand Up @@ -181,7 +163,7 @@ func runConfigGitRemoveCmd(cmd *cobra.Command, newClient ClientFactory) (err err
return
}

client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose})
client, done := newClient(ClientConfig{Verbose: cfg.Verbose})
defer done()

return client.RemovePAC(cmd.Context(), f, cfg.metadata)
Expand Down
7 changes: 2 additions & 5 deletions cmd/config_git_set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func NewConfigGitSetCmd(newClient ClientFactory) *cobra.Command {
directory or from the directory specified with --path.
`,
SuggestFor: []string{"add", "ad", "update", "create", "insert", "append"},
PreRunE: bindEnv("path", "builder", "builder-image", "image", "registry", "namespace", "git-provider", "git-url", "git-branch", "git-dir", "gh-access-token", "config-local", "config-cluster", "config-remote"),
PreRunE: bindEnv("path", "builder", "builder-image", "image", "registry", "git-provider", "git-url", "git-branch", "git-dir", "gh-access-token", "config-local", "config-cluster", "config-remote"),
RunE: func(cmd *cobra.Command, args []string) (err error) {
return runConfigGitSetCmd(cmd, newClient)
},
Expand Down Expand Up @@ -93,8 +93,6 @@ func NewConfigGitSetCmd(newClient ClientFactory) *cobra.Command {
type configGitSetConfig struct {
buildConfig // further embeds config.Global

Namespace string

GitProvider string
GitURL string
GitRevision string
Expand Down Expand Up @@ -127,7 +125,6 @@ func newConfigGitSetConfig(_ *cobra.Command) (c configGitSetConfig) {

c = configGitSetConfig{
buildConfig: newBuildConfig(),
Namespace: viper.GetString("namespace"),

GitURL: viper.GetString("git-url"),
GitRevision: viper.GetString("git-branch"),
Expand Down Expand Up @@ -307,7 +304,7 @@ func runConfigGitSetCmd(cmd *cobra.Command, newClient ClientFactory) (err error)
return
}

client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose}, fn.WithRegistry(cfg.Registry))
client, done := newClient(ClientConfig{Verbose: cfg.Verbose}, fn.WithRegistry(cfg.Registry))
defer done()

return client.ConfigurePAC(cmd.Context(), f, cfg.metadata)
Expand Down
13 changes: 7 additions & 6 deletions cmd/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@ import (
"errors"
"testing"

. "knative.dev/func/pkg/testing"
"knative.dev/func/pkg/utils"
)

// TestCreate_Execute ensures that an invocation of create with minimal settings
// and valid input completes without error; degenerate case.
func TestCreate_Execute(t *testing.T) {
_ = fromTempDirectory(t)
_ = FromTempDirectory(t)

cmd := NewCreateCmd(NewClient)
cmd.SetArgs([]string{"--language", "go", "myfunc"})
Expand All @@ -23,7 +24,7 @@ func TestCreate_Execute(t *testing.T) {
// TestCreate_NoRuntime ensures that an invocation of create must be
// done with a runtime.
func TestCreate_NoRuntime(t *testing.T) {
_ = fromTempDirectory(t)
_ = FromTempDirectory(t)

cmd := NewCreateCmd(NewClient)
cmd.SetArgs([]string{"myfunc"}) // Do not use test command args
Expand All @@ -38,7 +39,7 @@ func TestCreate_NoRuntime(t *testing.T) {
// TestCreate_WithNoRuntime ensures that an invocation of create must be
// done with one of the valid runtimes only.
func TestCreate_WithInvalidRuntime(t *testing.T) {
_ = fromTempDirectory(t)
_ = FromTempDirectory(t)

cmd := NewCreateCmd(NewClient)
cmd.SetArgs([]string{"--language", "invalid", "myfunc"})
Expand All @@ -53,7 +54,7 @@ func TestCreate_WithInvalidRuntime(t *testing.T) {
// TestCreate_InvalidTemplate ensures that an invocation of create must be
// done with one of the valid templates only.
func TestCreate_InvalidTemplate(t *testing.T) {
_ = fromTempDirectory(t)
_ = FromTempDirectory(t)

cmd := NewCreateCmd(NewClient)
cmd.SetArgs([]string{"--language", "go", "--template", "invalid", "myfunc"})
Expand All @@ -68,7 +69,7 @@ func TestCreate_InvalidTemplate(t *testing.T) {
// TestCreate_ValidatesName ensures that the create command only accepts
// DNS-1123 labels for function name.
func TestCreate_ValidatesName(t *testing.T) {
_ = fromTempDirectory(t)
_ = FromTempDirectory(t)

// Execute the command with a function name containing invalid characters and
// confirm the expected error is returned
Expand All @@ -84,7 +85,7 @@ func TestCreate_ValidatesName(t *testing.T) {
// TestCreate_ConfigOptional ensures that the system can be used without
// any additional configuration being required.
func TestCreate_ConfigOptional(t *testing.T) {
_ = fromTempDirectory(t)
_ = FromTempDirectory(t)

t.Setenv("XDG_CONFIG_HOME", t.TempDir())

Expand Down
Loading
Loading