Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(lint): updates golangci-lint to latest version #1074

Merged

Conversation

bartoszmajsak
Copy link
Contributor

@bartoszmajsak bartoszmajsak commented Jun 21, 2024

Description

Fixes

  • removed irrelevant //nolint comments (see config updates below)
  • converted unused func params to blank identifier when impl does not need it

New linters

perfsprint

  • disabled fmt.Sprintf checks for one argument string message, as it's been used in multiple places. I assume it's to favor readability and ease for further extension if more args needs
  • adjusted errors creation to use errors.New instead of fmt.Errorf when no-arg strings are used as messages

Configuration updates

  • configured revive to not complain about dot-imports for testing libraries
  • adjusted configuration due to changes in the structure (see skip-dirs)
  • some deprecated linters were removed from golangci-lint toolchain so they are now gone from the config too

How Has This Been Tested?

Screenshot or short clip

Merge criteria

  • You have read the contributors guide.
  • Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
  • 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

@bartoszmajsak bartoszmajsak removed the request for review from Sara4994 June 21, 2024 07:45
.golangci.yml Show resolved Hide resolved
@@ -71,6 +69,8 @@ linters:
- godox # https://github.com/opendatahub-io/opendatahub-operator/issues/699

issues:
exclude-dirs:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure i understand this part:
it was to skip linter running on the folder "apis"
not it runs on "apis" but do not report issue if found?

Copy link
Contributor Author

@bartoszmajsak bartoszmajsak Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved it to the new config block, as the old is deprecated. I think it's effectively the same.

But as we are touching on the apis exclusion. Maybe we should narrow the skip to auto-generated files instead. But I wonder if that's in scope of this PR. Maybe we should capture it as a task to review linter findings and decide which one to still exclude and also review the exclusion rules such as this. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a ticket https://issues.redhat.com/browse/RHOAIENG-481 maybe we should extend it to the new findings after this PR?

@bartoszmajsak
Copy link
Contributor Author

/retest

Fixes

- removed irrelavant //nolint comments (see config updates below)
- converted unused func params to blank identifier when impl does not
  need it

New linters

perfsprint

- disabled fmt.Sprintf checks for one element
- adjusted errors creation to use errors.New instead of fmt.Errorf when
  no-arg strings are used as messages

Configuration updates

- configured revive to not complain about dot-imports for testing
  libraries
- adjusted configuration yaml due to changes in the structure
  (skip-dirs)
- some deprecated linters were removed from golangci-lint toolchain so they are now gone from the config too
Copy link

openshift-ci bot commented Jun 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AjayJagan, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 9be146f into opendatahub-io:incubation Jun 24, 2024
8 checks passed
@@ -12,7 +13,7 @@ import (
operatorv1 "github.com/openshift/api/operator/v1"
"github.com/stretchr/testify/require"
autoscalingv1 "k8s.io/api/autoscaling/v1"
"k8s.io/apimachinery/pkg/api/errors"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be apierrs like in other places?

Copy link
Contributor Author

@bartoszmajsak bartoszmajsak Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seen both. Will check other places and align in a follow up PR. Though apierrors is a bit ambiguous.

Copy link
Contributor Author

@bartoszmajsak bartoszmajsak Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ykaliuta I opened a follow-up PR #1078

@bartoszmajsak bartoszmajsak deleted the lint-update branch June 24, 2024 18:12
@ykaliuta
Copy link
Contributor

I get

diff --git a/controllers/status/reporter.go b/controllers/status/reporter.go
index e5b3679ff326..2e70298616c7 100644
--- a/controllers/status/reporter.go
+++ b/controllers/status/reporter.go
@@ -1,4 +1,4 @@
-//nolint:ireturn //reason: return T which is expected to be satisfying client.Object interface
+
 package status
 
 import (

after make lint now

zdtsw pushed a commit to zdtsw-forking/rhods-operator that referenced this pull request Jun 26, 2024
Fixes

- removed irrelavant //nolint comments (see config updates below)
- converted unused func params to blank identifier when impl does not
  need it

New linters

perfsprint

- disabled fmt.Sprintf checks for one element
- adjusted errors creation to use errors.New instead of fmt.Errorf when
  no-arg strings are used as messages

Configuration updates

- configured revive to not complain about dot-imports for testing
  libraries
- adjusted configuration yaml due to changes in the structure
  (skip-dirs)
- some deprecated linters were removed from golangci-lint toolchain so they are now gone from the config too
openshift-merge-bot bot pushed a commit to red-hat-data-services/rhods-operator that referenced this pull request Jun 26, 2024
Fixes

- removed irrelavant //nolint comments (see config updates below)
- converted unused func params to blank identifier when impl does not
  need it

New linters

perfsprint

- disabled fmt.Sprintf checks for one element
- adjusted errors creation to use errors.New instead of fmt.Errorf when
  no-arg strings are used as messages

Configuration updates

- configured revive to not complain about dot-imports for testing
  libraries
- adjusted configuration yaml due to changes in the structure
  (skip-dirs)
- some deprecated linters were removed from golangci-lint toolchain so they are now gone from the config too
VaishnaviHire pushed a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Jul 24, 2024
Fixes

- removed irrelavant //nolint comments (see config updates below)
- converted unused func params to blank identifier when impl does not
  need it

New linters

perfsprint

- disabled fmt.Sprintf checks for one element
- adjusted errors creation to use errors.New instead of fmt.Errorf when
  no-arg strings are used as messages

Configuration updates

- configured revive to not complain about dot-imports for testing
  libraries
- adjusted configuration yaml due to changes in the structure
  (skip-dirs)
- some deprecated linters were removed from golangci-lint toolchain so they are now gone from the config too

(cherry picked from commit 9be146f)
VaishnaviHire pushed a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Jul 24, 2024
Fixes

- removed irrelavant //nolint comments (see config updates below)
- converted unused func params to blank identifier when impl does not
  need it

New linters

perfsprint

- disabled fmt.Sprintf checks for one element
- adjusted errors creation to use errors.New instead of fmt.Errorf when
  no-arg strings are used as messages

Configuration updates

- configured revive to not complain about dot-imports for testing
  libraries
- adjusted configuration yaml due to changes in the structure
  (skip-dirs)
- some deprecated linters were removed from golangci-lint toolchain so they are now gone from the config too

(cherry picked from commit 9be146f)
@bartoszmajsak
Copy link
Contributor Author

I get

diff --git a/controllers/status/reporter.go b/controllers/status/reporter.go
index e5b3679ff326..2e70298616c7 100644
--- a/controllers/status/reporter.go
+++ b/controllers/status/reporter.go
@@ -1,4 +1,4 @@
-//nolint:ireturn //reason: return T which is expected to be satisfying client.Object interface
+
 package status
 
 import (

after make lint now

@ykaliuta I hope this #1085 fixed the problem.

@ykaliuta
Copy link
Contributor

@ykaliuta I hope this #1085 fixed the problem.

yep. Thank you!

ykaliuta added a commit to ykaliuta/opendatahub-operator that referenced this pull request Aug 1, 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]>
ykaliuta added a commit to ykaliuta/opendatahub-operator that referenced this pull request Aug 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]>
ykaliuta added a commit to ykaliuta/opendatahub-operator that referenced this pull request Aug 16, 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]>
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]>
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]>
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 (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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants