Skip to content

Conversation

@ncaak
Copy link
Contributor

@ncaak ncaak commented Jun 2, 2023

summary

  • Extract the hardcoded disabled checks to a configmap default configuration (yaml)
  • Fix ConfigMap watcher which in the future will take care of dynamic configuration

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Jun 2, 2023

@ncaak: This pull request references DVO-110 which is a valid jira issue.

In response to this:

summary

  • Extract the hardcoded disabled checks to a configmap default configuration (yaml)
  • Fix ConfigMap watcher which in the future will take care of dynamic configuration

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link

openshift-ci bot commented Jun 2, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci
Copy link

openshift-ci bot commented Jun 2, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncaak

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-ci openshift-ci bot added the approved label Jun 2, 2023
@ncaak ncaak force-pushed the DVO-110/move-disable-checks-configmap branch from 9cb4959 to 03ddc2c Compare June 2, 2023 17:37
@ncaak
Copy link
Contributor Author

ncaak commented Jun 2, 2023

/test lint

@ncaak
Copy link
Contributor Author

ncaak commented Jun 8, 2023

/test lint

# Takes precedence over doNotAutoAddDefaults, if both are set.
addAllBuiltIn: true
exclude: ["access-to-create-pods", "access-to-secrets", "cluster-admin-role-binding", "default-service-account", "deprecated-service-account-field", "docker-sock", "drop-net-raw-capability", "env-var-secret", "exposed-services", "latest-tag", "mismatching-selector", "no-extensions-v1beta", "no-liveness-probe", "no-read-only-root-fs", "no-readiness-probe", "no-rolling-update-strategy", "privileged-ports", "read-secret-from-env-var", "required-annotation-email", "required-label-owner", "sensitive-host-mounts", "ssh-port", "unsafe-proc-mount", "use-namespace", "wildcard-in-rules", "writable-host-mount"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding to default ConfigMap the disable checks from pkg/validations/validation_engine.go file where they were hardcoded.
This is still being not dynamically changed, but it removes hardcode configuration and allow more flexibility.

@ncaak ncaak marked this pull request as ready for review June 8, 2023 15:28
@openshift-ci openshift-ci bot requested review from apahim and tremes June 8, 2023 15:28
@ncaak ncaak changed the title WIP DVO-110: Move disable checks to configmap DVO-110: Move disable checks to configmap Jun 8, 2023
@tremes
Copy link
Contributor

tremes commented Jun 12, 2023

@ncaak it looks like it would be good to update the README file too.

cmw.ch <- struct{}{}
cfg, err := cmw.getKubeLinterConfig(newCm.Data[configMapDataAccess])
if err != nil {
fmt.Printf("Error: unmarshalling configmap data: %s", err.Error())
Copy link
Contributor

@tremes tremes Jun 12, 2023

Choose a reason for hiding this comment

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

I think we should add some logger to the configmap_watcher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll take a look and reuse logger for error messages as well.

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2023

Codecov Report

Merging #253 (88b7b6c) into master (d0931ef) will decrease coverage by 0.72%.
The diff coverage is 24.13%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
- Coverage   43.40%   42.68%   -0.72%     
==========================================
  Files          23       24       +1     
  Lines         940     1012      +72     
==========================================
+ Hits          408      432      +24     
- Misses        497      539      +42     
- Partials       35       41       +6     
Impacted Files Coverage Δ
pkg/validations/validation_engine.go 41.60% <0.00%> (-25.34%) ⬇️
pkg/controller/configmap_watcher.go 23.07% <38.88%> (+9.74%) ⬆️

... and 2 files with indirect coverage changes


The `exclude` property takes precedence over the `include` property. If a particular check is in both collections, it will be excluded by default.

The `exclude` property can work in conjunction with `addAllBuiltIn` set to `true` in a blacklisting fashion. All checks will be triggered and only the checks passed in `exclude` will be ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could also say what the default configuration is (when the configmap doesn't exist).

@tremes
Copy link
Contributor

tremes commented Jun 27, 2023

I reviewed and experimented a little bit with this one. Thanks.
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jun 27, 2023
@openshift-merge-robot openshift-merge-robot merged commit 1482629 into app-sre:master Jun 27, 2023
@ncaak ncaak deleted the DVO-110/move-disable-checks-configmap branch February 7, 2025 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants