Drop the use dprotaso/controller-tools fork #15664

dprotaso · 2024-12-27T16:50:00Z

Keeping the fork up to date is not really tenable and it's fairly complex. We now simply manipulate the resulting CRD yaml after generation

dprotaso · 2024-12-27T16:50:53Z

cc @skonto

I need to revisit some new attributes that were added so marking WIP for now

codecov · 2024-12-27T16:55:04Z

Codecov Report

Attention: Patch coverage is 4.36187% with 592 lines in your changes missing coverage. Please review.

Project coverage is 80.75%. Comparing base (79f8c3f) to head (b7673dd).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/schema-tweak/overrides.go	0.00%	341 Missing ⚠️
cmd/schema-tweak/main.go	0.00%	162 Missing ⚠️
cmd/schema-tweak/yaml.go	0.00%	88 Missing ⚠️
pkg/apis/config/features.go	96.42%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15664      +/-   ##
==========================================
- Coverage   83.48%   80.75%   -2.74%     
==========================================
  Files         219      222       +3     
  Lines       17432    18024     +592     
==========================================
+ Hits        14553    14555       +2     
- Misses       2504     3096     +592     
+ Partials      375      373       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/apis/config/features.go

dprotaso · 2024-12-28T16:43:49Z

/assign @skonto

This should unblock your k8s bump

dprotaso · 2024-12-30T23:19:06Z

/retest

skonto · 2025-01-07T09:56:22Z

@dprotaso hi, could you update the PR?

cmd/schema-tweak/main.go

Keeping the fork up to date is not really tenable and it's fairly complex. We now simply manipulate the resulting CRD yaml after generation

cmd/schema-tweak/overrides.go

skonto · 2025-01-08T17:47:11Z

Adding a new flag & attribute:

diff --git a/cmd/schema-tweak/overrides.go b/cmd/schema-tweak/overrides.go
index c111d0770..d4fd19bcb 100644
--- a/cmd/schema-tweak/overrides.go
+++ b/cmd/schema-tweak/overrides.go
@@ -243,6 +243,9 @@ func revSpecOverrides(prefixPath string) []entry {
                }, {
                        name: "hostPath",
                        flag: "kubernetes.podspec-hostpath",
+               }, {
+                       name: "csi",
+                       flag: "kubernetes.podspec-csi",
                }},
        }, {
                path: "volumes.secret",

works as expected generating the new field in all related resource files:

+++ b/config/core/300-resources/configuration.yaml
@@ -1163,6 +1163,10 @@ spec:
                                     description: optional specify whether the ConfigMap or its keys must be defined
                                     type: boolean
                                 x-kubernetes-map-type: atomic
+                              csi:
+                                description: This is accessible behind a feature flag - kubernetes.podspec-csi
+                                type: object
+                                x-kubernetes-preserve-unknown-fields: true
                               emptyDir:
                                 description: |-
                                   This is accessible behind a feature flag - kubernetes.podspec-emptydir

Adding a new attribute (not behind a flag):

+++ b/cmd/schema-tweak/overrides.go
@@ -120,6 +120,7 @@ func revSpecOverrides(prefixPath string) []entry {
                        "terminationMessagePolicy",
                        "volumeMounts",
                        "workingDir",
+                       "resizePolicy",
                ),
        }, {

generates the expected output:

+++ b/config/core/300-resources/configuration.yaml
@@ -624,6 +624,27 @@ spec:
                                       More info: https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle#container-probes
                                     type: integer
                                     format: int32
+                              resizePolicy:
+                                description: Resources resize policy for the container.
+                                type: array
+                                items:
+                                  description: ContainerResizePolicy represents resource resize policy for the container.
+                                  type: object
+                                  required:
+                                    - resourceName
+                                    - restartPolicy
+                                  properties:
+                                    resourceName:
+                                      description: |-
+                                        Name of the resource to which this resource resize policy applies.
+                                        Supported values: cpu, memory.
+                                      type: string
+                                    restartPolicy:
+                                      description: |-
+                                        Restart policy to apply when specified resource is resized.
+                                        If not specified, it defaults to NotRequired.
+                                      type: string
+                                x-kubernetes-list-type: atomic
                               resources:
                                 description: |-
                                   Compute Resources required by this container.
@@ -1163,6 +1184,10 @@ spec:

Should we add tests in case we need to fix something, instead of doing it manually?

skonto · 2025-01-08T17:55:17Z

cmd/schema-tweak/main.go

+		log.Fatalln("failed to read CRD directory", err)
+	}
+
+	for _, file := range files {


Although it is fast already could this be done concurrently? 🤔

I don't think it's worth the complexity this would introduce - eg. right now I just log.Fatalf which quits the program.

Doing it concurrently would require changing it to return errors etc.

cmd/schema-tweak/main.go

dprotaso · 2025-01-08T19:44:55Z

/retest

dprotaso · 2025-01-08T22:22:52Z

kourier-tls failure

/retest

dprotaso · 2025-01-09T00:22:40Z

again kourier-tls
/retest

skonto · 2025-01-09T12:02:46Z

cmd/schema-tweak/main.go

+}
+
+func applyOverrideEntry(node *yaml.Node, entry entry) {
+	for _, segment := range strings.Split(entry.path, ".") {


I guess there is no way to quickly reference a path (its node) so we have to iterate.

skonto · 2025-01-09T12:03:10Z

/lgtm
/approve

knative-prow · 2025-01-09T12:03:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, skonto

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:

~~OWNERS~~ [dprotaso,skonto]
~~pkg/apis/OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow bot requested review from izabelacg and skonto December 27, 2024 16:50

dprotaso commented Dec 28, 2024

View reviewed changes

pkg/apis/config/features.go Show resolved Hide resolved

dprotaso changed the title ~~[wip] Drop the use dprotaso/controller-tools fork~~ Drop the use dprotaso/controller-tools fork Dec 28, 2024

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 28, 2024

knative-prow bot assigned skonto Dec 28, 2024

dprotaso mentioned this pull request Dec 28, 2024

Update k8s min and deps to 1.30 knative/pkg#3124

Merged

dprotaso force-pushed the schema-tweak branch 2 times, most recently from 9f09651 to a8e7a70 Compare December 30, 2024 20:33

skonto added this to the v1.17.0 milestone Jan 7, 2025

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2025

skonto reviewed Jan 8, 2025

View reviewed changes

cmd/schema-tweak/main.go Show resolved Hide resolved

dprotaso added 5 commits January 8, 2025 11:51

Drop the use dprotaso/controller-tools fork

30aec0a

Keeping the fork up to date is not really tenable and it's fairly complex. We now simply manipulate the resulting CRD yaml after generation

go mod tidy

fb9afb5

Prune the allowed scaleTargetRef fields

7cef5e8

allow containers.privileged

e165e5a

update overrides to include hostPath feature flag

def8273

dprotaso force-pushed the schema-tweak branch from a8e7a70 to def8273 Compare January 8, 2025 17:03

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 8, 2025

skonto reviewed Jan 8, 2025

View reviewed changes

cmd/schema-tweak/overrides.go Outdated Show resolved Hide resolved

skonto reviewed Jan 8, 2025

View reviewed changes

cmd/schema-tweak/main.go Outdated Show resolved Hide resolved

refactor function for readability

93beafd

skonto reviewed Jan 8, 2025

View reviewed changes

cmd/schema-tweak/main.go Show resolved Hide resolved

dprotaso added 3 commits January 8, 2025 14:09

refactor feature key constants to prevent typos

fa4c92a

drop schema patch config file

6616ad3

close encoder prior to writing the file

b7673dd

skonto reviewed Jan 9, 2025

View reviewed changes

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2025

knative-prow bot merged commit 3216ab3 into knative:main Jan 9, 2025
68 checks passed

dprotaso deleted the schema-tweak branch January 9, 2025 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop the use dprotaso/controller-tools fork #15664

Drop the use dprotaso/controller-tools fork #15664

dprotaso commented Dec 27, 2024

dprotaso commented Dec 27, 2024

codecov bot commented Dec 27, 2024 •

edited

Loading

dprotaso commented Dec 28, 2024

dprotaso commented Dec 30, 2024

skonto commented Jan 7, 2025 •

edited

Loading

skonto commented Jan 8, 2025 •

edited

Loading

skonto Jan 8, 2025

dprotaso Jan 8, 2025

dprotaso commented Jan 8, 2025

dprotaso commented Jan 8, 2025

dprotaso commented Jan 9, 2025

skonto Jan 9, 2025

skonto commented Jan 9, 2025

knative-prow bot commented Jan 9, 2025

Drop the use dprotaso/controller-tools fork #15664

Drop the use dprotaso/controller-tools fork #15664

Conversation

dprotaso commented Dec 27, 2024

dprotaso commented Dec 27, 2024

codecov bot commented Dec 27, 2024 • edited Loading

Codecov Report

dprotaso commented Dec 28, 2024

dprotaso commented Dec 30, 2024

skonto commented Jan 7, 2025 • edited Loading

skonto commented Jan 8, 2025 • edited Loading

skonto Jan 8, 2025

Choose a reason for hiding this comment

dprotaso Jan 8, 2025

Choose a reason for hiding this comment

dprotaso commented Jan 8, 2025

dprotaso commented Jan 8, 2025

dprotaso commented Jan 9, 2025

skonto Jan 9, 2025

Choose a reason for hiding this comment

skonto commented Jan 9, 2025

knative-prow bot commented Jan 9, 2025

codecov bot commented Dec 27, 2024 •

edited

Loading

skonto commented Jan 7, 2025 •

edited

Loading

skonto commented Jan 8, 2025 •

edited

Loading