add ConditionsMatch function

[sbryzak]

Refactor to move conditions testing functions from the test package to the conditions package so that they may be used outside of tests

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.12%. Comparing base (ed50ce5) to head (783e16f).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #397      +/-   ##
==========================================
+ Coverage   77.98%   78.12%   +0.13%     
==========================================
  Files          47       47              
  Lines        1958     1970      +12     
==========================================
+ Hits         1527     1539      +12     
  Misses        373      373              
  Partials       58       58              

Files	Coverage Δ
pkg/condition/condition.go	85.22% <100.00%> (+3.79%)	⬆️
pkg/test/condition.go	30.00% <100.00%> (-11.67%)	⬇️

[alexeykazakov]

Looks good. However we need unit tests :)

[metlos]

Apart from the comments below, could you please provide a little bit of context for why this new function is needed? codeready-toolchain/registration-service#411 (which is linked from SANDBOX-554) was closed more than a month ago and does not use this change.

[sbryzak]

@metlos thanks for the review - this PR is not actually new code, it's just a refactoring to move existing code to a different package in order to allow it to be used outside of the tests. It is already used extensively throughout the dev sandbox codebase and very well tested, so i'm hesitant to change any of it at this point.

[MatousJobanek]

@sbryzak you are right - this code was used in unit tests, but could you please provide more context for why you need to use it in production? As @metlos mentioned in his comment, there is no point in checking the message content in production.

[alexeykazakov]

This is supposed to be used in this PR: codeready-toolchain/host-operator#988

[alexeykazakov]

Regarding the Message thing. I agree that matching Message in runtime code is at least questionable. However it may make sense in tests. Depending on how it's actually used in tests. So I would be safer to keep it for the tests.
We could do something like this:

func containsCondition(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition, ignoreMessage bool) bool {
    ...
}

func ContainsCondition(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition) bool {
    return containsConditions(conditions, contains, true)
}

func ContainsConditionWithMessage(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition) bool {
    return containsConditions(conditions, contains, false)
}

And in the test package:

func ContainsCondition(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition) bool {
    return ContainsConditionWithMessage(conditions, contains)
}

[MatousJobanek]

Sure, it makes completely sense to match the message in the tests, but why moving it out of the pkg/test/ package? All these functions that are there can be (and are) already used by unit tests in host-operator see https://github.com/codeready-toolchain/host-operator/blob/423ebbae4392a3752562f9f975e8db3532b31b2d/controllers/usersignup/usersignup_controller_test.go#L33
Let's keep it in the pkg/test/ package to make it clear that it's supposed to be used in tests only. There is no need to match the message in the production code.

[alexeykazakov]

Let's keep it in the pkg/test/ package to make it clear that it's supposed to be used in tests only. There is no need to match the message in the production code.

Shane wants to use it in runtime too. See the new predicate code in the host operator PR: https://github.com/codeready-toolchain/host-operator/pull/988/files#diff-686778ddb77570eda2cfd3b717a28573e5b4399c35d7f85936143fe0feff0dedR52

[MatousJobanek]

ok, this is exactly what I was looking for, thanks for the link.
TBH, I don't see any reason for using any predicate in this particular case at all. I mean, what is the point of the predicate? But let's move this discussion to the other PR.

[alexeykazakov]

Looks good overall. Just a couple of minor comments regarding missing function descriptions. And also it looks like you can add a bit more tests.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
5.1% Duplication on New Code

See analysis details on SonarCloud