Refactor endpoint and policy packages to separate test code from production code #35384
Conversation
|
Commit 680e375 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
680e375 to
25ce082
Compare
|
/test |
There was a problem hiding this comment.
Thanks @roykharman for taking care of this.
The first two commits look pretty straightforward to me. I've additionally cced the original authors who had introduced the usage of testing.T in these methods, to get their opinion as well. Based on the original commit messages, the idea there was to make it clear that the given methods are intended for testing purposes only, and to prevent using them in production code.
I'm a bit more doubtful about the kvstore changes. Although I agree with the overall intent, the current changes are leading to duplicated code (which will deverge over time), and the need of exposing methods that were intended for testing only. I've left a few suggestions on how to try to refactor those to simplify the logic and replace the usage of some internal methods, but I'm not totally sure if it will be possible to achieve a really good solution without a significant refactoring of the kvstore package. Similarly, I'm also not really sure whether avoiding to import the testing package in production code really outweighs the extra complexity introduced here, but I guess that will become clearer after addressing the initial suggestions.
Please also note that the PR currently contains a merge commit, which are not allowed in Cilium. Please rebase your PR onto the main branch to fix the conflicts and remove it.
|
Hi @giorio94, Thank you for your very detailed review. I do agree the added complexity outweighs the benefit. We saw this issue and thought it would be a straight forward place to contribute to a better code base.
let's see what the original authors think about this and proceed from there.
Valid point, can be done on a separate stage.
Probably happened during rebase, @roykharman will fix it. Thank you for you very detailed and fast review. |
25ce082 to
696e20b
Compare
|
/test |
|
Thanks for the changes, overall look good to me. Before approving it, could you please
|
|
Thank you for the feedback, @giorio94! I'll go ahead and fix the remaining linting errors by renaming the occurrences I missed and I'll also update the PR title and description to reflect that the kvstore package is no longer modified, as suggested. |
…ilium#34069) - Removed an unused `testing.TB` parameter from the `NewTestEndpointWithState` function in `endpoint.go`. - This parameter was not being utilized within the function, and its removal simplifies the function signature. - The `testing.TB` parameter seems redundant in this context, as the function does not rely on it for any testing behavior. - Removing this parameter also eliminates the need for the `testing` package import in production code, contributing to the resolution of issue cilium#34069 by reducing the unnecessary weight and surface area of the production build. Signed-off-by: Roy <roykharman@gmail.com> Reviewed-by: Sameeh Jubran <sameeh.j@gmail.com>
- Removed an unused `testing.TB` parameter from a function in `l4.go`. - This parameter was not being used within the function and was likely a leftover placeholder. - By removing this unused parameter, the function is simplified and no longer depends on the `testing` package. - This change contributes to resolving issue cilium#34069 by removing the unnecessary inclusion of the Go built-in "testing" package from production builds, reducing both weight and surface area. Signed-off-by: Roy <roykharman@gmail.com> Reviewed-by: Sameeh Jubran <sameeh.j@gmail.com>
- Renamed two functions in l4.go to 'TestingOnly*' prefix to clarify their use for testing only. - Added a note to NewTestEndpointWithState indicating it is for testing purposes. - This change contributes to resolving issue cilium#34069 by making test-specific code more explicit. Signed-off-by: Roy <roykharman@gmail.com> Reviewed-by: Sameeh Jubran <sameeh.j@gmail.com>
…#34069) - Renamed remaining Equals and Diff functions to TestingOnlyEquals and TestingOnlyDiff. - Fixed all linting errors. Signed-off-by: Roy <roykharman@gmail.com>
Head branch was pushed to by a user without write access
696e20b to
e37eef1
Compare
|
/test |
|
(I've slightly edited the title and description of the PR to remove the remaining references to the kvstore package, and prevent the linked issue from being automatically closed when this PR gets merged, given that is not yet fully addressed. |
|
Thank you all for the reviews and approvals, @giorio94, @nathanjsweet and @aanm. Before moving forward, I wanted to check if any additional changes are required or if there is anything else that needs addressing. I also noticed that two checks are currently failing. While the pull request has been approved, I’m unsure if these failures are related to my changes. Could you please provide guidance on whether I need to address these failing checks? Thank you again for your feedback and assistance. |
The two failures are unrelated, and caused by flakes --- I've just retriggered them. No action is required on your side. |
This pull request refactors the policy and endpoint packages to improve code clarity, reduce unnecessary complexity, and ensure clear distinction between production and test-specific code, addressing issue #34069.
Key Changes:
[1] Removed Unused testing.TB Parameters:
In the l4.go file, the unused testing.TB parameters were removed from test-related functions. This reduces unnecessary complexity and ensures these functions are cleaner and not tied to the testing package when it's not needed.
[2] Renamed Test-Specific Functions:
Renamed the Equals and Diff functions in l4.go to TestingOnlyEquals and TestingOnlyDiff, respectively. This change explicitly marks these functions as intended for testing purposes only, helping to prevent accidental use in production code.
[3] Clarifying Note in NewTestEndpointWithState:
Added a comment to the NewTestEndpointWithState function in the endpoint package, clearly indicating that this function is intended solely for testing purposes. This helps future developers quickly understand the function's purpose.
[4] Linting and Error Fixes:
Fixed several linting errors and addressed undefined references caused by the initial renaming of test-specific functions. All necessary functions were renamed appropriately, and the code passes linting checks.
Challenges Faced:
While we initially aimed to refactor parts of the kvstore package, the decision was ultimately made to focus on renaming functions and removing unnecessary parameters in the policy and endpoint packages. The final solution ensures that test-specific functions are clearly marked, simplifying future maintenance and reducing the risk of test code being accidentally used in production environments.
Regarding the kvstore package we attempted to move test-specific code into separate test files to better isolate testing logic from production code. However, this approach caused several undefined reference errors between packages, complicating access to internal functions. We also considered moving the test logic to utility files, but this did not resolve the accessibility issues.
Towards: #34069
Signed-off-by: Roy roykharman@gmail.com
Reviewed-by: Sameeh Jubran sameeh.j@gmail.com