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. |
6e6b96d
to
ef3e1b2
Compare
c3917e5
to
a3a11f2
Compare
ca1df7f
to
fb30c90
Compare
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. |
89bd283
to
d578935
Compare
d578935
to
b03d2d8
Compare
| @@ -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.
|
[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 |
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