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] Fix enablement variable setting priority #1578

Merged
merged 5 commits into from
Jul 22, 2021
Merged

[chore] Fix enablement variable setting priority #1578

merged 5 commits into from
Jul 22, 2021

Conversation

ccfishk
Copy link
Contributor

@ccfishk ccfishk commented Jul 22, 2021

What this PR does / why we need it:
KIC starts a controller only CRD exists on the cluster though user off the CR plugin through enablement environment variable.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
Enablement environment variable flag exposed user (i.e --controller-kongclusterplugin, --controller-knativeingress)should have highest priority.

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@ccfishk ccfishk requested a review from a team as a code owner July 22, 2021 16:49
@codecov
Copy link

codecov bot commented Jul 22, 2021

Codecov Report

Merging #1578 (20f7acd) into next (208733f) will decrease coverage by 11.73%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             next    #1578       +/-   ##
===========================================
- Coverage   51.51%   39.78%   -11.74%     
===========================================
  Files          93       91        -2     
  Lines        8737     8327      -410     
===========================================
- Hits         4501     3313     -1188     
- Misses       3926     4842      +916     
+ Partials      310      172      -138     
Flag Coverage Δ
integration-test ?
unit-test 39.78% <0.00%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
railgun/internal/ctrlutils/ingress-status.go 0.00% <ø> (-66.00%) ⬇️
railgun/internal/manager/controllerdef.go 0.00% <0.00%> (-88.49%) ⬇️
railgun/internal/manager/run.go 0.00% <0.00%> (-50.00%) ⬇️
railgun/internal/manager/ingressapi.go 0.00% <0.00%> (-97.06%) ⬇️
railgun/internal/ctrlutils/utils.go 0.00% <0.00%> (-93.75%) ⬇️
railgun/internal/manager/setup.go 0.00% <0.00%> (-66.28%) ⬇️
railgun/internal/diagnostics/server.go 0.00% <0.00%> (-61.54%) ⬇️
railgun/internal/mgrutils/reports.go 0.00% <0.00%> (-55.56%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 208733f...20f7acd. Read the comment docs.

@ccfishk ccfishk changed the base branch from main to next July 22, 2021 16:50
@ccfishk ccfishk temporarily deployed to Configure ci July 22, 2021 16:50 Inactive
@github-actions
Copy link

Licenses differ between commit 18cc5df89a4bac4af012f7de543c8d571335ed71 and base:

+++ pr_licenses.csv	2021-07-22 16:51:57.083562640 +0000
@@ -1,3 +1,8 @@
+github.com/Masterminds/goutils,https:/Masterminds/goutils/blob/master/LICENSE.txt,Apache-2.0
+github.com/Masterminds/semver,https:/Masterminds/semver/blob/master/LICENSE.txt,MIT
+github.com/Masterminds/sprig,https:/Masterminds/sprig/blob/master/LICENSE.txt,MIT
+github.com/alecthomas/template,https:/alecthomas/template/blob/master/LICENSE,BSD-3-Clause
+github.com/alecthomas/units,https:/alecthomas/units/blob/master/COPYING,MIT
 github.com/beorn7/perks/quantile,https:/beorn7/perks/blob/master/quantile/LICENSE,MIT
 github.com/blang/semver/v4,https:/blang/semver/blob/master/v4/LICENSE,MIT
 github.com/bombsimon/logrusr,https:/bombsimon/logrusr/blob/master/LICENCE,MIT
@@ -7,6 +12,7 @@
 github.com/eapache/channels,https:/eapache/channels/blob/master/LICENSE,MIT
 github.com/eapache/queue,https:/eapache/queue/blob/master/LICENSE,MIT
 github.com/evanphx/json-patch,https:/evanphx/json-patch/blob/master/LICENSE,BSD-3-Clause
+github.com/evanphx/json-patch/v5,https:/evanphx/json-patch/blob/master/v5/LICENSE,BSD-3-Clause
 github.com/fatih/color,https:/fatih/color/blob/master/LICENSE.md,MIT
 github.com/fsnotify/fsnotify,https:/fsnotify/fsnotify/blob/master/LICENSE,BSD-3-Clause
 github.com/ghodss/yaml,https:/ghodss/yaml/blob/master/LICENSE,MIT
@@ -25,16 +31,19 @@
 github.com/hashicorp/golang-lru,https:/hashicorp/golang-lru/blob/master/LICENSE,MPL-2.0
 github.com/hashicorp/hcl,https:/hashicorp/hcl/blob/master/LICENSE,MPL-2.0
 github.com/hexops/gotextdiff,https:/hexops/gotextdiff/blob/master/LICENSE,BSD-3-Clause
+github.com/huandu/xstrings,https:/huandu/xstrings/blob/master/LICENSE,MIT
 github.com/imdario/mergo,https:/imdario/mergo/blob/master/LICENSE,BSD-3-Clause
 github.com/json-iterator/go,https:/json-iterator/go/blob/master/LICENSE,MIT
 github.com/magiconair/properties,https:/magiconair/properties/blob/master/LICENSE.md,BSD-2-Clause
 github.com/mattn/go-colorable,https:/mattn/go-colorable/blob/master/LICENSE,MIT
 github.com/mattn/go-isatty,https:/mattn/go-isatty/blob/master/LICENSE,MIT
 github.com/matttproud/golang_protobuf_extensions/pbutil,https:/matttproud/golang_protobuf_extensions/blob/master/pbutil/LICENSE,Apache-2.0
+github.com/mitchellh/copystructure,https:/mitchellh/copystructure/blob/master/LICENSE,MIT
 github.com/mitchellh/mapstructure,https:/mitchellh/mapstructure/blob/master/LICENSE,MIT
+github.com/mitchellh/reflectwalk,https:/mitchellh/reflectwalk/blob/master/LICENSE,MIT
 github.com/modern-go/concurrent,https:/modern-go/concurrent/blob/master/LICENSE,Apache-2.0
 github.com/modern-go/reflect2,https:/modern-go/reflect2/blob/master/LICENSE,Apache-2.0
-github.com/pelletier/go-toml,https:/pelletier/go-toml/blob/master/LICENSE,MIT
+github.com/pelletier/go-toml,https:/pelletier/go-toml/blob/master/LICENSE,Apache-2.0
 github.com/pkg/errors,https:/pkg/errors/blob/master/LICENSE,BSD-2-Clause
 github.com/prometheus/client_golang/prometheus,https:/prometheus/client_golang/blob/master/prometheus/LICENSE,Apache-2.0
 github.com/prometheus/client_model/go,https:/prometheus/client_model/blob/master/go/LICENSE,Apache-2.0
@@ -64,7 +73,7 @@
 go.uber.org/zap,Unknown,MIT
 gomodules.xyz/jsonpatch/v2,Unknown,Apache-2.0
 google.golang.org/protobuf,Unknown,BSD-3-Clause
-gopkg.in/evanphx/json-patch.v4,Unknown,BSD-3-Clause
+gopkg.in/alecthomas/kingpin.v2,Unknown,MIT
 gopkg.in/go-playground/pool.v3,Unknown,MIT
 gopkg.in/inf.v0,Unknown,BSD-3-Clause
 gopkg.in/ini.v1,Unknown,Apache-2.0
@@ -82,5 +91,5 @@
 knative.dev/networking/pkg,Unknown,Apache-2.0
 knative.dev/pkg,Unknown,Apache-2.0
 sigs.k8s.io/controller-runtime,Unknown,Apache-2.0
-sigs.k8s.io/structured-merge-diff/v4/value,Unknown,Apache-2.0
+sigs.k8s.io/structured-merge-diff/v4,Unknown,Apache-2.0
 sigs.k8s.io/yaml,Unknown,MIT```

@ccfishk ccfishk added bug Something isn't working area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. labels Jul 22, 2021
@ccfishk ccfishk self-assigned this Jul 22, 2021
@ccfishk ccfishk temporarily deployed to Configure ci July 22, 2021 17:04 Inactive
@ccfishk ccfishk temporarily deployed to Configure ci July 22, 2021 17:04 Inactive
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

I think that reducing log noise when controllers are specifically disabled is overall good, so I have no blockers for this change.

I do however have several comments, and before this PR is merged I would ask that each gets resolved, either by:

  1. merging the provided suggestion
  2. creating a follow up

Before KIC 2.0 releases we need to harden how this stuff works, and improve the documentation so that we're consistent and so contributors and end-users don't get confused.

railgun/internal/ctrlutils/ingress-status.go Show resolved Hide resolved
railgun/internal/manager/controllerdef.go Show resolved Hide resolved
railgun/internal/manager/controllerdef.go Show resolved Hide resolved
railgun/internal/manager/controllerdef.go Outdated Show resolved Hide resolved
railgun/internal/manager/controllerdef.go Show resolved Hide resolved
railgun/internal/manager/controllerdef.go Outdated Show resolved Hide resolved
@shaneutt
Copy link
Contributor

In retrospect, I created a follow up item anyway because we're going to need it in any case:

#1580

While all the suggestions I made I feel are still valid, given this follow up item if you're feeling crunched for time please feel free to merge with OR without them at your discretion.

@ccfishk ccfishk temporarily deployed to Configure ci July 22, 2021 18:58 Inactive
@ccfishk ccfishk temporarily deployed to Configure ci July 22, 2021 18:58 Inactive
@ccfishk ccfishk merged commit dcabd38 into next Jul 22, 2021
@ccfishk ccfishk deleted the fix-priority branch July 22, 2021 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. bug Something isn't working ci/license/changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants