chore: update golangci-lint to v1.60.2, fix misleading test #1195
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lburgazzoli
force-pushed
the
golangci-lint-v1.60.x
branch
from
a814da1
to
a818218
Compare
Signed-off-by: Luca Burgazzoli <[email protected]>
lburgazzoli
force-pushed
the
golangci-lint-v1.60.x
branch
from
a818218
to
c941c42
Compare
lburgazzoli
changed the title
asanzgom
approved these changes
Aug 21, 2024
lburgazzoli
commented
Aug 21, 2024
| if *reconciledDep.Spec.Replicas != int32(expectedReplica) { | ||
| return fmt.Errorf("failed to revert back replicas : expect to be %v but got %v", expectedReplica, *reconciledDep.Spec.Replicas) | ||
| } | ||
| if len(appDeployments.Items) != 1 { |
There was a problem hiding this comment.
@VaishnaviHire @zdtsw I've reworked this test because I think it was incorrect, like if there were no deployments, then the test would have succeeded without actually testing anything.
lburgazzoli
force-pushed
the
golangci-lint-v1.60.x
branch
from
559a9ae
to
6611686
Compare
lburgazzoli
changed the title
zdtsw
approved these changes
Aug 21, 2024
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asanzgom, zdtsw 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 |
openshift-merge-bot
bot
merged commit 6acf1db
into
opendatahub-io:incubation
8 checks passed
zdtsw
pushed a commit
to zdtsw-forking/rhods-operator
that referenced
this pull request
Aug 29, 2024
…hub-io#1195) * chore: update golangci-lint to v1.60.2 Signed-off-by: Luca Burgazzoli <[email protected]> * chore: rework UpdateComponentReconcile test --------- Signed-off-by: Luca Burgazzoli <[email protected]> (cherry picked from commit 6acf1db)
openshift-merge-bot bot
pushed a commit
to red-hat-data-services/rhods-operator
that referenced
this pull request
Aug 30, 2024
…hub-io#1195) * chore: update golangci-lint to v1.60.2 Signed-off-by: Luca Burgazzoli <[email protected]> * chore: rework UpdateComponentReconcile test --------- Signed-off-by: Luca Burgazzoli <[email protected]> (cherry picked from commit 6acf1db)
ykaliuta
added a commit
to ykaliuta/opendatahub-operator
that referenced
this pull request
Sep 2, 2024
Jira: https://issues.redhat.com/browse/RHOAIENG-4268 This patch reverts 5288015 ("Revert "DSC, DSCI: add validating webhook (opendatahub-io#711)"") * webhook: add initial skeleton Originally it was generated with ```operator-sdk create webhook --group datasciencecluster --version v1 --kind DataScienceCluster --programmatic-validation``` but webhook.Validator interface (like described in the kubebuilder book[1]) does not work well for the purpose of the webhook due to needs to access openshift cluster (client.Client) to check existing instances of DSC. So, direct implementation of Handler was done inspired by [2] and odh-notebooks implementation [3]. Move it from api package closer to controllers as in [3] as well since it's not DataScienceCluster or DSCInitialization extention anymore. Amend webhook_suite_test.go's path to configs accordingly. Fix linter issues in webhook_suite_test.go: - disable ssl check; - move to package webhook_test certmanager files removed too due to usage of OpenShift service serving certificates[4] (see also service.beta.openshift.io/inject-cabundle annotation in config/webhook/kustomization.yaml). Add webhook generation to `make manifests` target so webhook/manifests.yaml is generated with it. Since DSCI creation now requires webhook it should be delayed after manager started. Move it to a closure and add it to the manager for run with Add() API. It requires explicit declaration of the interface variable otherwise complains about type mismatch for the function literal. [1] https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation [2] https://book-v1.book.kubebuilder.io/beyond_basics/sample_webhook.html [3] https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/controllers/notebook_webhook.go [4] https://docs.openshift.com/container-platform/4.9/security/certificates/service-serving-certificate.html Signed-off-by: Yauheni Kaliuta <[email protected]> * webhook: implement one instance enforcing The webhook is written with the idea to handle both Create and Update requests (configured in config/webhook/manifests.yaml), but at the moment only duplication check on Create is implemented. Implements the logic which is done now on reconcile time [1] (same for DSCI). It checks for 0 instances, not 1, since when the webhook is running the object has not been created yet. Means if it's 1 then it handles request to create a second one. It could be probably possible to use generics but does not make a lot of sense for such a simple case. Closes: opendatahub-io#693 [1] https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/controllers/datasciencecluster/datasciencecluster_controller.go#L98 Signed-off-by: Yauheni Kaliuta <[email protected]> * tests: add tests to check duplication blocking Add both envtest and e2e tests of a second DataScienceCluster instance creation blocking. envtest's one is a part of webhook test suite. e2e: Add `name` parameter to setupDSCInstance() function to reuse it. Use require.Error() as the assertion, shorter and more straight logic than implementing it in the test itself. Add e2e test to check DSCInitialization similar way. Signed-off-by: Yauheni Kaliuta <[email protected]> * tests: e2e: refactor duplication tests in more abstract way Factor out common code using Unstructured/List objects. Change structure to remind more prepare/action/assert. Use "require" features when appropriate. Signed-off-by: Yauheni Kaliuta <[email protected]> --------- Signed-off-by: Yauheni Kaliuta <[email protected]> chore(webhook): (opendatahub-io#870) - add testcase on DSCI - remove kubebuilder marker not needed - remove checks on instance number in existing controllers - re-generate bundle - we do not act on update but we keep it on webhook for now Signed-off-by: Wen Zhou <[email protected]> fix uncommented tests/e2e/dsc_creation_test.go with a line from 9be146f ("chore(lint): updates to latest version (opendatahub-io#1074)") Signed-off-by: Yauheni Kaliuta <[email protected]> webhook: partial: upgrade: controller-runtime and code change accordingly Partial application of already applied 03c1abc ("upgrade: controller-runtime and code change accordingly (opendatahub-io#1189)") Webhook related changes. Signed-off-by: Yauheni Kaliuta <[email protected]> tests: e2e: fix requireInstalled to check actuall list emptiness Partial backport of 6acf1db ("chore: update golangci-lint to v1.60.2, fix misleading test (opendatahub-io#1195)") Signed-off-by: Yauheni Kaliuta <[email protected]>
ykaliuta
added a commit
to ykaliuta/opendatahub-operator
that referenced
this pull request
Sep 12, 2024
Jira: https://issues.redhat.com/browse/RHOAIENG-4268 This patch reverts 5288015 ("Revert "DSC, DSCI: add validating webhook (opendatahub-io#711)"") * webhook: add initial skeleton Originally it was generated with ```operator-sdk create webhook --group datasciencecluster --version v1 --kind DataScienceCluster --programmatic-validation``` but webhook.Validator interface (like described in the kubebuilder book[1]) does not work well for the purpose of the webhook due to needs to access openshift cluster (client.Client) to check existing instances of DSC. So, direct implementation of Handler was done inspired by [2] and odh-notebooks implementation [3]. Move it from api package closer to controllers as in [3] as well since it's not DataScienceCluster or DSCInitialization extention anymore. Amend webhook_suite_test.go's path to configs accordingly. Fix linter issues in webhook_suite_test.go: - disable ssl check; - move to package webhook_test certmanager files removed too due to usage of OpenShift service serving certificates[4] (see also service.beta.openshift.io/inject-cabundle annotation in config/webhook/kustomization.yaml). Add webhook generation to `make manifests` target so webhook/manifests.yaml is generated with it. Since DSCI creation now requires webhook it should be delayed after manager started. Move it to a closure and add it to the manager for run with Add() API. It requires explicit declaration of the interface variable otherwise complains about type mismatch for the function literal. [1] https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation [2] https://book-v1.book.kubebuilder.io/beyond_basics/sample_webhook.html [3] https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/controllers/notebook_webhook.go [4] https://docs.openshift.com/container-platform/4.9/security/certificates/service-serving-certificate.html Signed-off-by: Yauheni Kaliuta <[email protected]> * webhook: implement one instance enforcing The webhook is written with the idea to handle both Create and Update requests (configured in config/webhook/manifests.yaml), but at the moment only duplication check on Create is implemented. Implements the logic which is done now on reconcile time [1] (same for DSCI). It checks for 0 instances, not 1, since when the webhook is running the object has not been created yet. Means if it's 1 then it handles request to create a second one. It could be probably possible to use generics but does not make a lot of sense for such a simple case. Closes: opendatahub-io#693 [1] https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/controllers/datasciencecluster/datasciencecluster_controller.go#L98 Signed-off-by: Yauheni Kaliuta <[email protected]> * tests: add tests to check duplication blocking Add both envtest and e2e tests of a second DataScienceCluster instance creation blocking. envtest's one is a part of webhook test suite. e2e: Add `name` parameter to setupDSCInstance() function to reuse it. Use require.Error() as the assertion, shorter and more straight logic than implementing it in the test itself. Add e2e test to check DSCInitialization similar way. Signed-off-by: Yauheni Kaliuta <[email protected]> * tests: e2e: refactor duplication tests in more abstract way Factor out common code using Unstructured/List objects. Change structure to remind more prepare/action/assert. Use "require" features when appropriate. Signed-off-by: Yauheni Kaliuta <[email protected]> --------- Signed-off-by: Yauheni Kaliuta <[email protected]> chore(webhook): (opendatahub-io#870) - add testcase on DSCI - remove kubebuilder marker not needed - remove checks on instance number in existing controllers - re-generate bundle - we do not act on update but we keep it on webhook for now Signed-off-by: Wen Zhou <[email protected]> fix uncommented tests/e2e/dsc_creation_test.go with a line from 9be146f ("chore(lint): updates to latest version (opendatahub-io#1074)") Signed-off-by: Yauheni Kaliuta <[email protected]> webhook: partial: upgrade: controller-runtime and code change accordingly Partial application of already applied 03c1abc ("upgrade: controller-runtime and code change accordingly (opendatahub-io#1189)") Webhook related changes. Signed-off-by: Yauheni Kaliuta <[email protected]> tests: e2e: fix requireInstalled to check actuall list emptiness Partial backport of 6acf1db ("chore: update golangci-lint to v1.60.2, fix misleading test (opendatahub-io#1195)") Signed-off-by: Yauheni Kaliuta <[email protected]> networkpolicy: allow connections from webhook Allow connection to the operator pod from host network, marked with label `policy-group.network.openshift.io/host-network: ""` https://access.redhat.com/solutions/7008681 Signed-off-by: Yauheni Kaliuta <[email protected]>
ykaliuta
added a commit
to ykaliuta/opendatahub-operator
that referenced
this pull request
Sep 13, 2024
Jira: https://issues.redhat.com/browse/RHOAIENG-4268 This patch reverts 5288015 ("Revert "DSC, DSCI: add validating webhook (opendatahub-io#711)"") * webhook: add initial skeleton Originally it was generated with ```operator-sdk create webhook --group datasciencecluster --version v1 --kind DataScienceCluster --programmatic-validation``` but webhook.Validator interface (like described in the kubebuilder book[1]) does not work well for the purpose of the webhook due to needs to access openshift cluster (client.Client) to check existing instances of DSC. So, direct implementation of Handler was done inspired by [2] and odh-notebooks implementation [3]. Move it from api package closer to controllers as in [3] as well since it's not DataScienceCluster or DSCInitialization extention anymore. Amend webhook_suite_test.go's path to configs accordingly. Fix linter issues in webhook_suite_test.go: - disable ssl check; - move to package webhook_test certmanager files removed too due to usage of OpenShift service serving certificates[4] (see also service.beta.openshift.io/inject-cabundle annotation in config/webhook/kustomization.yaml). Add webhook generation to `make manifests` target so webhook/manifests.yaml is generated with it. Since DSCI creation now requires webhook it should be delayed after manager started. Move it to a closure and add it to the manager for run with Add() API. It requires explicit declaration of the interface variable otherwise complains about type mismatch for the function literal. [1] https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation [2] https://book-v1.book.kubebuilder.io/beyond_basics/sample_webhook.html [3] https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/controllers/notebook_webhook.go [4] https://docs.openshift.com/container-platform/4.9/security/certificates/service-serving-certificate.html Signed-off-by: Yauheni Kaliuta <[email protected]> * webhook: implement one instance enforcing The webhook is written with the idea to handle both Create and Update requests (configured in config/webhook/manifests.yaml), but at the moment only duplication check on Create is implemented. Implements the logic which is done now on reconcile time [1] (same for DSCI). It checks for 0 instances, not 1, since when the webhook is running the object has not been created yet. Means if it's 1 then it handles request to create a second one. It could be probably possible to use generics but does not make a lot of sense for such a simple case. Closes: opendatahub-io#693 [1] https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/controllers/datasciencecluster/datasciencecluster_controller.go#L98 Signed-off-by: Yauheni Kaliuta <[email protected]> * tests: add tests to check duplication blocking Add both envtest and e2e tests of a second DataScienceCluster instance creation blocking. envtest's one is a part of webhook test suite. e2e: Add `name` parameter to setupDSCInstance() function to reuse it. Use require.Error() as the assertion, shorter and more straight logic than implementing it in the test itself. Add e2e test to check DSCInitialization similar way. Signed-off-by: Yauheni Kaliuta <[email protected]> * tests: e2e: refactor duplication tests in more abstract way Factor out common code using Unstructured/List objects. Change structure to remind more prepare/action/assert. Use "require" features when appropriate. Signed-off-by: Yauheni Kaliuta <[email protected]> --------- Signed-off-by: Yauheni Kaliuta <[email protected]> chore(webhook): (opendatahub-io#870) - add testcase on DSCI - remove kubebuilder marker not needed - remove checks on instance number in existing controllers - re-generate bundle - we do not act on update but we keep it on webhook for now Signed-off-by: Wen Zhou <[email protected]> fix uncommented tests/e2e/dsc_creation_test.go with a line from 9be146f ("chore(lint): updates to latest version (opendatahub-io#1074)") Signed-off-by: Yauheni Kaliuta <[email protected]> webhook: partial: upgrade: controller-runtime and code change accordingly Partial application of already applied 03c1abc ("upgrade: controller-runtime and code change accordingly (opendatahub-io#1189)") Webhook related changes. Signed-off-by: Yauheni Kaliuta <[email protected]> tests: e2e: fix requireInstalled to check actuall list emptiness Partial backport of 6acf1db ("chore: update golangci-lint to v1.60.2, fix misleading test (opendatahub-io#1195)") Signed-off-by: Yauheni Kaliuta <[email protected]> networkpolicy: allow connections from webhook Allow connection to the operator pod from host network, marked with label `policy-group.network.openshift.io/host-network: ""` https://access.redhat.com/solutions/7008681 Signed-off-by: Yauheni Kaliuta <[email protected]>
openshift-merge-bot bot
pushed a commit
to red-hat-data-services/rhods-operator
that referenced
this pull request
Sep 13, 2024
Jira: https://issues.redhat.com/browse/RHOAIENG-4268 This patch reverts 5288015 ("Revert "DSC, DSCI: add validating webhook (#711)"") * webhook: add initial skeleton Originally it was generated with ```operator-sdk create webhook --group datasciencecluster --version v1 --kind DataScienceCluster --programmatic-validation``` but webhook.Validator interface (like described in the kubebuilder book[1]) does not work well for the purpose of the webhook due to needs to access openshift cluster (client.Client) to check existing instances of DSC. So, direct implementation of Handler was done inspired by [2] and odh-notebooks implementation [3]. Move it from api package closer to controllers as in [3] as well since it's not DataScienceCluster or DSCInitialization extention anymore. Amend webhook_suite_test.go's path to configs accordingly. Fix linter issues in webhook_suite_test.go: - disable ssl check; - move to package webhook_test certmanager files removed too due to usage of OpenShift service serving certificates[4] (see also service.beta.openshift.io/inject-cabundle annotation in config/webhook/kustomization.yaml). Add webhook generation to `make manifests` target so webhook/manifests.yaml is generated with it. Since DSCI creation now requires webhook it should be delayed after manager started. Move it to a closure and add it to the manager for run with Add() API. It requires explicit declaration of the interface variable otherwise complains about type mismatch for the function literal. [1] https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation [2] https://book-v1.book.kubebuilder.io/beyond_basics/sample_webhook.html [3] https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/controllers/notebook_webhook.go [4] https://docs.openshift.com/container-platform/4.9/security/certificates/service-serving-certificate.html Signed-off-by: Yauheni Kaliuta <[email protected]> * webhook: implement one instance enforcing The webhook is written with the idea to handle both Create and Update requests (configured in config/webhook/manifests.yaml), but at the moment only duplication check on Create is implemented. Implements the logic which is done now on reconcile time [1] (same for DSCI). It checks for 0 instances, not 1, since when the webhook is running the object has not been created yet. Means if it's 1 then it handles request to create a second one. It could be probably possible to use generics but does not make a lot of sense for such a simple case. Closes: #693 [1] https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/controllers/datasciencecluster/datasciencecluster_controller.go#L98 Signed-off-by: Yauheni Kaliuta <[email protected]> * tests: add tests to check duplication blocking Add both envtest and e2e tests of a second DataScienceCluster instance creation blocking. envtest's one is a part of webhook test suite. e2e: Add `name` parameter to setupDSCInstance() function to reuse it. Use require.Error() as the assertion, shorter and more straight logic than implementing it in the test itself. Add e2e test to check DSCInitialization similar way. Signed-off-by: Yauheni Kaliuta <[email protected]> * tests: e2e: refactor duplication tests in more abstract way Factor out common code using Unstructured/List objects. Change structure to remind more prepare/action/assert. Use "require" features when appropriate. Signed-off-by: Yauheni Kaliuta <[email protected]> --------- Signed-off-by: Yauheni Kaliuta <[email protected]> chore(webhook): (opendatahub-io#870) - add testcase on DSCI - remove kubebuilder marker not needed - remove checks on instance number in existing controllers - re-generate bundle - we do not act on update but we keep it on webhook for now Signed-off-by: Wen Zhou <[email protected]> fix uncommented tests/e2e/dsc_creation_test.go with a line from 9be146f ("chore(lint): updates to latest version (opendatahub-io#1074)") Signed-off-by: Yauheni Kaliuta <[email protected]> webhook: partial: upgrade: controller-runtime and code change accordingly Partial application of already applied 03c1abc ("upgrade: controller-runtime and code change accordingly (opendatahub-io#1189)") Webhook related changes. Signed-off-by: Yauheni Kaliuta <[email protected]> tests: e2e: fix requireInstalled to check actuall list emptiness Partial backport of 6acf1db ("chore: update golangci-lint to v1.60.2, fix misleading test (opendatahub-io#1195)") Signed-off-by: Yauheni Kaliuta <[email protected]> networkpolicy: allow connections from webhook Allow connection to the operator pod from host network, marked with label `policy-group.network.openshift.io/host-network: ""` https://access.redhat.com/solutions/7008681 Signed-off-by: Yauheni Kaliuta <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While I've fixed a lint finding on one of the e2e tests, I also reworked it a bit (in a separate commit) since there were a condition that was not properly taken into account.
I could split this PR in two separate ones if it makes things more clear.
Description
How Has This Been Tested?
Screenshot or short clip
Merge criteria
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).The developer has manually tested the changes and verified that the changes work