fix: test and namespace isolation

[lkingland]

Changes

• :broom: removes now-unnecessary Namespace member from structs
• :bug: several small bugfixes
• :broom: general cleanup

tl;dr

Namespace as a struct member only works under fairly tight circumstances. Recent functional additions invalidate this assumption. This PR therefore removes namespace as a structure state member, adding it to method signatures where appropriate. This keeps things flat and stupid-simple.

Background

The initial design was to encapsulate the effective namespace within a fn.Client instance (and concrete implementations of the constituent interfaces like Deployer, Lister, Remover etc) by providing it as a constructor argument. All operations conducted via the client instance would then be targeted at that namespace. Clients, being lightweight structures, can be instantiated easily, one for each namespace. This allowed the target namespace to only be a concern during initial instantiation, with all following operations being able to ignore this internal state. However, this assumption turned out to be incorrect for a couple of reasons: cross-namespace operations and developer familiarity.

As new methods and options were added to the Client, there came to exist a few operations whose target namespace was either ambiguous or undefined. For example deploying a function to a new namespace requires removing it from the old. What, then, is the internal state of the namespace member? This broke the encapsulation of that complexity.

Second, and perhaps more practical, is the expectation by users of the client instance (most of whom are at least familiar with the underlying Kubernetes implementation) expect namespace to be a primary concern.

These two conspired to result in several operations which expected namespace as a per-request argument. This of course caused all logic to then treat the struct member as more of a fallback or default.

This is unnecessary complexity. If a member like namespace can be completely internalized and removed from all method arguments, only being considered during instantiation, then it has succeeded in reducing complexy. If this member "leaks" into any of the struct methods, then it is no longer assisting,but instead causing more complexity than just adding it to the set of expected arguments.

Therefore, this PR accepts that namespace is an expected argument in most cases, removing it from constructors and any state.

/kind bug /kind cleanup

Fixes #

Release Note

Fixed Function namespace resolution in some edge cases.

Docs

[knative-prow[bot]]

@lkingland: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

In response to this:

Changes

• :broom: removes now-unnecessary Deployer Namespace member
• :bug: fix test failure if environment already has an active k8s context
• :broom: FromTempDirectory moved to shared testing package

/kind bug

/kind

Fixes #

Release Note

Fixed Function namespace resolution in some edge cases.

Docs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 65.38462% with 117 lines in your changes are missing coverage. Please review.

Project coverage is 64.49%. Comparing base (b97d841) to head (98dc85b). Report is 92 commits behind head on main.

Files	Patch %	Lines
pkg/pipelines/tekton/pipelines_provider.go	46.55%	25 Missing and 6 partials :warning:
pkg/functions/client.go	74.60%	12 Missing and 4 partials :warning:
pkg/pipelines/tekton/pipelines_pac_provider.go	0.00%	12 Missing :warning:
cmd/describe.go	71.42%	4 Missing and 4 partials :warning:
pkg/mock/pipelines_provider.go	46.15%	3 Missing and 4 partials :warning:
cmd/deploy.go	66.66%	3 Missing and 3 partials :warning:
cmd/delete.go	80.76%	4 Missing and 1 partial :warning:
cmd/root.go	70.58%	2 Missing and 3 partials :warning:
pkg/mock/deployer.go	50.00%	3 Missing and 1 partial :warning:
pkg/pipelines/tekton/resources.go	50.00%	2 Missing and 2 partials :warning:
... and 11 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2187      +/-   ##
==========================================
+ Coverage   64.21%   64.49%   +0.28%     
==========================================
  Files         108      127      +19     
  Lines       13918    11454    -2464     
==========================================
- Hits         8937     7387    -1550     
+ Misses       4108     3160     -948     
- Partials      873      907      +34     

Flag	Coverage Δ
e2e-test	38.14% <43.70%> (+0.76%)	:arrow_up:
e2e-test-oncluster	31.19% <41.05%> (+0.69%)	:arrow_up:
e2e-test-oncluster-runtime	27.49% <35.43%> (?)
e2e-test-runtime-go	26.09% <29.80%> (?)
e2e-test-runtime-node	27.12% <30.46%> (?)
e2e-test-runtime-python	27.05% <29.80%> (?)
e2e-test-runtime-quarkus	27.14% <29.80%> (?)
e2e-test-runtime-rust	26.19% <29.80%> (?)
e2e-test-runtime-springboot	26.22% <29.80%> (?)
e2e-test-runtime-typescript	27.21% <30.46%> (?)
integration-tests	50.13% <60.94%> (+0.27%)	:arrow_up:
unit-tests	49.15% <44.67%> (?)
unit-tests-macos-latest	?
unit-tests-ubuntu-latest	?
unit-tests-windows-latest	?

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[knative-prow[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, matzew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [lkingland,matzew]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[matzew]

@lkingland any updates?

[lkingland]

@lkingland any updates?

Could use another /LGTM presuming the tests pass!

[gauron99]

/lgtm

[gauron99]

/lgtm