Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

charts: split properties for webhook and manager #1261

Merged
merged 1 commit into from
Jul 31, 2020
Merged

charts: split properties for webhook and manager #1261

merged 1 commit into from
Jul 31, 2020
+58 −22

Conversation

hectorj2f
Copy link
Contributor

What this PR does / why we need it:

We should split the webhook properties from the manager properties.
Also we should expose the env properties for the chart as well.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Closes #1259

Special notes for your reviewer:

@hectorj2f hectorj2f self-assigned this Jul 29, 2020
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 29, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2020
RainbowMango
RainbowMango reviewed Jul 30, 2020
View reviewed changes
charts/kubefed/values.yaml
@@ -6,19 +6,36 @@
##
controllermanager:
Copy link
Contributor

Choose a reason for hiding this comment

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

(Not sure if it works.) Just a guess, would it be better to rename controllermanager to other common names such as component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might imply to change the chart name. I don't know if we really want to do that now.

@RainbowMango
Copy link
Contributor

Just a nit, otherwise LGTM.

irfanurrehman
irfanurrehman reviewed Jul 30, 2020
View reviewed changes
charts/kubefed/values.yaml Outdated
requests:
cpu: 100m
memory: 64Mi

clusterAvailableDelay:
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't these properties also applicable only to controller?

Copy link
Contributor Author

@hectorj2f hectorj2f Jul 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

These are part of controllermanager main chart, perhaps I can move them up to easily identify the scope for manager and webhook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved them up so it is more clear the purpose of the properties manager and webhook. To me, properties such as clusterAvailableDelay are defining a more generic behavior to the whole kubefed control-plane.

@hectorj2f hectorj2f force-pushed the hectorj2f/split_controllermanager_properties branch from 54462c4 to db6b5e6 Compare July 30, 2020 10:22
@hectorj2f hectorj2f force-pushed the hectorj2f/split_controllermanager_properties branch from db6b5e6 to 12b1666 Compare July 30, 2020 12:05
@RainbowMango
Copy link
Contributor

LGTM. I'd like to give a chance to @irfanurrehman for final approval.

@irfanurrehman
Copy link
Contributor

Thanks for doing this @hectorj2f.
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hectorj2f, irfanurrehman

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 [hectorj2f,irfanurrehman]

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

@k8s-ci-robot k8s-ci-robot merged commit 31d26fe into kubernetes-retired:master Jul 31, 2020
@hectorj2f
Copy link
Contributor Author

Thanks for the reviews.

@hectorj2f hectorj2f deleted the hectorj2f/split_controllermanager_properties branch July 31, 2020 08:21
@jimmidyson jimmidyson mentioned this pull request Aug 5, 2020
Merged
@pregnor pregnor mentioned this pull request Aug 19, 2020
Merged
2 tasks
pregnor added a commit to banzaicloud/pipeline that referenced this pull request Aug 24, 2020
@pregnor
chore(kubefed): updated version (0.3.1-banzai.1->)0.4.0
304512b 
Note: kubefed brought some other transitive dependency updates with
itself. Checked the release notes, haven't seen potentially breaking
changes.

Note: kubernetes-retired/kubefed#1261 this PR
changed the official kubefed chart value handling by putting the
common image values under a `controller` and `webhook` fields in
the main values file to provide separated control over the two.

controllermanager.image          -> controllermanager.controller.image
                                 -> controllermanager.webhook.image

controllermanager.imagePullPolicy-> controllermanager.controller.imagePullPolicy
                                 -> controllermanager.webhook.imagePullPolicy

controllermanager.replicaCount   -> controllermanager.controller.replicaCount
                                 -> controllermanager.webhook.replicaCount

controllermanager.repository     -> controllermanager.controller.repository
                                 -> controllermanager.webhook.repository

controllermanager.tag            -> controllermanager.controller.tag
                                 -> controllermanager.webhook.tag
pregnor added a commit to banzaicloud/pipeline that referenced this pull request Aug 24, 2020
@pregnor
chore(kubefed): updated version (0.3.1-banzai.1->)0.4.0
4ae4c4d 
Note: kubefed brought some other transitive dependency updates with
itself. Checked the release notes, haven't seen potentially breaking
changes.

Note: kubernetes-retired/kubefed#1261 this PR
changed the official kubefed chart value handling by putting the
common image values under a `controller` and `webhook` fields in
the main values file to provide separated control over the two.

controllermanager.image          -> controllermanager.controller.image
                                 -> controllermanager.webhook.image

controllermanager.imagePullPolicy-> controllermanager.controller.imagePullPolicy
                                 -> controllermanager.webhook.imagePullPolicy

controllermanager.replicaCount   -> controllermanager.controller.replicaCount
                                 -> controllermanager.webhook.replicaCount

controllermanager.repository     -> controllermanager.controller.repository
                                 -> controllermanager.webhook.repository

controllermanager.tag            -> controllermanager.controller.tag
                                 -> controllermanager.webhook.tag
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jimmidyson jimmidyson Awaiting requested review from jimmidyson

@shashidharatd shashidharatd Awaiting requested review from shashidharatd

@irfanurrehman irfanurrehman Awaiting requested review from irfanurrehman

@RainbowMango RainbowMango Awaiting requested review from RainbowMango

Assignees

@hectorj2f hectorj2f

@irfanurrehman irfanurrehman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

chart: expose the env property for the kubefed components in values.yaml
4 participants
@hectorj2f @RainbowMango @irfanurrehman @k8s-ci-robot