fix: test and namespace isolation #2187
Conversation
|
@lkingland: The label(s) In response to this:
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. |
knative-prow
bot
added
approved
lkingland
requested review from
gauron99
and removed request for
maximilien and
navidshaikh
knative-prow
bot
added
the
do-not-merge/work-in-progress
lkingland
force-pushed
the
test-isolation
branch
from
6e6b96d to
ef3e1b2
Compare
knative-prow
bot
added
size/XXL
lkingland
added
the
kind/cleanup
lkingland
force-pushed
the
test-isolation
branch
2 times, most recently
from
c3917e5 to
a3a11f2
Compare
knative-prow-robot
added
the
needs-rebase
lkingland
force-pushed
the
test-isolation
branch
from
ca1df7f to
fb30c90
Compare
knative-prow-robot
removed
the
needs-rebase
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
lkingland
force-pushed
the
test-isolation
branch
4 times, most recently
from
89bd283 to
d578935
Compare
lkingland
force-pushed
the
test-isolation
branch
from
d578935 to
b03d2d8
Compare
knative-prow
bot
removed
the
do-not-merge/work-in-progress
lkingland
requested review from
matzew,
matejvasek and
gauron99
and removed request for
jrangelramos and
nainaz
| @@ -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"), | |||
There was a problem hiding this comment.
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)
| @@ -28,7 +28,7 @@ No local files are deleted. | |||
| {{rootCmdUse}} delete | |||
|
|
|||
| # Undeploy the function 'myfunc' in namespace 'apps' | |||
| {{rootCmdUse}} delete -n apps myfunc | |||
| {{rootCmdUse}} delete myfunc --namespace apps | |||
There was a problem hiding this comment.
does this need release notes? Mirgation? or the like?
There was a problem hiding this comment.
These two commands are equivalent. I was just making the documentation use the long-form name for (hopefully) better clarity.
knative-prow
bot
added
the
do-not-merge/hold
|
[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:
Approvers can indicate their approval by writing |
|
@lkingland any updates? |
Could use another /LGTM presuming the tests pass! |
|
/lgtm |
|
/lgtm |
lkingland
removed
the
do-not-merge/hold
Changes
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
Docs