Closed rexagod closed 6 days ago
The code change looks good to me, pending the resolution of https://github.com/openshift/library-go/pull/1823#discussion_r1793445225
/approve /assign @p0lyn0mial
for lgtm
@rexagod In general, I have a couple of suggestions for you to consider:
It seems that ApplyUnstructuredResourceImproved
method could be made unexported/private. Do you agree?
The calling functions of the applyUnstructuredResourceImproved
pass nil for defaultingFunc
and equalityChecker
, so I would like to remove these arguments from applyUnstructuredResourceImproved.
I would like to remove the Fail-fast if the resource versions differ
code, since we merge into the existing object, not the required one.
I would like to remove an extra copy from the ensureGenericsSpec
method.
I would like to create a new method, EnsureObjectMetaForUnstructured
, where EnsureObjectMeta
is currently located.
I would like to move the code that merges metadata into this new function (code from line 105
to line 144
).
What do you think?
It seems that ApplyUnstructuredResourceImproved method could be made unexported/private. Do you agree?
The motivation to move to an unstructured.Unstructured
approach was to accommodate for resources that do not have their own Apply<Resource>
methods exposed. The ones in the library right now are only there for the sake of backwards-compatibility.
The calling functions of the applyUnstructuredResourceImproved pass nil for defaultingFunc and equalityChecker, so I would like to remove these arguments from applyUnstructuredResourceImproved.
To refer to my same reasoning as above, those are nil
for the exposed resource-dedicated methods as that was the original behavior. However, when using ApplyUnstructuredResourceImproved
for either (a) unknown resources (ones that do not have a dedicated method here), or (b) more control over the caching mechanism, those are still needed to leverage what the machinery has to offer. For instance, the exposed methods do not use a cache as well, to preserve older behavior, but nonetheless, something the user may still need.
I would like to remove the Fail-fast if the resource versions differ code, since we merge into the existing object, not the required one.
Done.
I would like to remove an extra copy from the ensureGenericsSpec method.
Done.
I would like to create a new method, EnsureObjectMetaForUnstructured, where EnsureObjectMeta is currently located. I would like to move the code that merges metadata into this new function (code from line 105 to line 144).
Done.
Thank you for the thorough review, @p0lyn0mial. I'd like to point out that I've refactored monitoring.go
code to make it more consistent and readable (for instance, the ensureGenericSpec
signature changes), in addition to the much-needed refactors you pointed out owing to the older code smells that existed pre-#1575. PLMK if I missed something.
Is ApplyUnstructuredResourceImproved
method used outside of the library-go? If not, I would make it private and remove the nil parameters. I would only add them when they are truly needed.
Also, could you please remove fixup! regression:
from the commit messages?
The ApplyUnstructuredResourceImproved
is used in CMO, for the reasons stated in my comment here: https://github.com/openshift/library-go/pull/1823#issuecomment-2406986360.
I'll squash the fixup!
commits.
@rexagod Thank you very much for your work - much appreciated!
would you be willing to open a PR similar to this one so we can be sure that the changes introduced in this PR are working as expected?
Sure, will do! 🙂
ok, the ci/prow/e2e-aws-ovn-upgrade
on the PR was green. Please rebase this PR and let's merge it.
@rexagod I think you need to add GO_TEST_PACKAGES :=./test/e2e-monitoring/...
to the test-e2e-monitoring
target.
mhm, I think that ci/prow/e2e-monitoring
isn't running TestResourceVersionApplication
.
Is it when you run make test-e2e-monitoring
on your local machine ?
Is it when you run make test-e2e-monitoring on your local machine ?
@p0lyn0mial The target was a placeholder to get the release
PR in. I've amended that to run the tests now. However, test-e2e-encryption
needs a similar refactor as that's still a placeholder IIUC. I believe this can be done in a different PR?
/test e2e-aws-encryption
test-e2e-encryption
is not a placeholder, the rule is invoking https://github.com/openshift/build-machinery-go/blob/5725581bdf8f467ee8fbd394f081a10df670c1ec/make/targets/golang/test-unit.mk#L12 on https://github.com/openshift/library-go/tree/master/test/e2e-encryption.
Ah, good to know why we specify GO_TEST_PACKAGES
, however, I'm not sure how frequently this runs? /test e2e-aws-encryption
(and local invocation) returns a make: Nothing to be done for 'test-e2e-encryption'
at the moment, which led me to believe it to be a placeholder, but it seems the actual go test
invocation resides in the shared Makefile repository that affects the way /test e2e-aws-encryption
works (the fact that it can only be run using that source)?
I ask this because I believe we would've caught the issue on master
if it did anytime soon (currently reproducible).
┌[rexagod@nebuchadnezzar] [/dev/ttys003] [master]
└[~/repositories/work/library-go]> gd upstream/master
┌[rexagod@nebuchadnezzar] [/dev/ttys003] [master]
└[~/repositories/work/library-go]> go vet ./...
# github.com/openshift/library-go/test/e2e-encryption
# [github.com/openshift/library-go/test/e2e-encryption]
vet: test/e2e-encryption/encryption_test.go:85:119: not enough arguments in call to genericoperatorclient.NewClusterScopedOperatorClient
have (*rest.Config, schema.GroupVersionResource)
want (clock.PassiveClock, *rest.Config, schema.GroupVersionResource, schema.GroupVersionKind, genericoperatorclient.OperatorSpecExtractorFunc, genericoperatorclient.OperatorStatusExtractorFunc)
We may benefit from incorporating the invocation command downstream so /test ...
and local invocations can run the tests (and running them in a similar manner in the relevant test infrastructure projects).
let's try with https://github.com/openshift/library-go/pull/1836 - i have run it on my local machine:
❯ make test-e2e-monitoring
go test -mod=vendor -race ./test/e2e-monitoring/...
ok github.com/openshift/library-go/test/e2e-monitoring (cached)
i'm waiting for the ci job to finish, if it works then we need something similar for the e2e-aws-encryption
.
/test e2e-aws-encryption (and local invocation) returns a make: Nothing to be done for 'test-e2e-encryption' at the moment
Oh I see, I missed that. If you add the following line, it should fix it:
diff --git a/Makefile b/Makefile
index 43276499c..b466e269a 100644
--- a/Makefile
+++ b/Makefile
@@ -24,6 +24,7 @@ verify-podnetworkconnectivitychecks:
$(MAKE) -C pkg/operator/connectivitycheckcontroller verify
test-e2e-encryption: GO_TEST_PACKAGES :=./test/e2e-encryption/...
+test-e2e-encryption: test-unit
.PHONY: test-e2e-encryption
test-e2e-monitoring
It must have been missed in the past
Hmm actually it reminded me of something and it looks like I had a PR for it that never merged: https://github.com/openshift/library-go/pull/1470
@rexagod https://github.com/openshift/library-go/pull/1836 worked, we have two options we can merge it or you can pull the changes to your pr.
i'm waiting for the ci job to finish, if it works then we need something similar for the e2e-aws-encryption.
Since my contextual test-e2e-encryption
-knowledge is limited and the fact that we've identified at-least a couple of issues [1][2] around the test, I'd reckon it'd be better if one of the previous authors with some spare cycles can take a look.
Otherwise, I'll revisit this as soon as I can find some.
Since my contextual test-e2e-encryption-knowledge is limited and the fact that we've identified at-least a couple of issues [1][2] around the test, I'd reckon it'd be better if one of the previous authors with some spare cycles can take a look.
@dgrisonnet has a pr for it - https://github.com/openshift/library-go/pull/1470
@rexagod: all tests passed!
Full PR test history. Your PR dashboard.
/lgtm
@rexagod many thanks for the PR.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: deads2k, p0lyn0mial, rexagod
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Set resource version for all
monitoring.go
GVRs. Earlier, this was not done by the machinery responsible for it, which caused manifest applications that had no resource version present to encounter the following error:This patch addresses that regression, which was introduced originally in #1575.
EDIT: I believe this is not a regression since 4.17 did not include #1575.