You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2021/03/11 15:39:59 UTC
[GitHub] [camel-k] squakez opened a new pull request #2114: feat(operator): toleration install flag
squakez opened a new pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114
* Refactored toleration trait to create Tolerations in a util method
* Refactored OperatorOrCollect method to ease readability
* Added toleration configuration in OLM and non OLM installations
<!-- Description -->
With this PR we introduce the possibility to setup `Toleration`s during installation phase. The same configuration applies to `OLM` and non `OLM` installation.
<!--
Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE".
You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
the text is added to the right section of the release notes.
-->
**Release Note**
```release-note
feat(operator): toleration install flag
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez removed a comment on pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
squakez removed a comment on pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#issuecomment-808126604
@astefanutti I think this is now good for merge. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
astefanutti commented on pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#issuecomment-797379234
@squakez what would you think of applying the others customizers from the operator Deployment, like the monitoring port, to the CSV as well? I think you've almost done all the work with that PR, and it would avoid having some asymmetric options.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on a change in pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
squakez commented on a change in pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#discussion_r594188898
##########
File path: pkg/util/kubernetes/util.go
##########
@@ -228,3 +232,37 @@ func ResolveValueSource(ctx context.Context, client k8sclient.Reader, namespace
return "", nil
}
+
+// GetTolerations build an array of Tolerations from an array of string
+func GetTolerations(taints []string) ([]corev1.Toleration, error) {
Review comment:
Makes sense, just fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
squakez commented on pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#issuecomment-808126604
@astefanutti I think this is now good for merge. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on a change in pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
astefanutti commented on a change in pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#discussion_r593185058
##########
File path: pkg/util/kubernetes/util.go
##########
@@ -228,3 +232,37 @@ func ResolveValueSource(ctx context.Context, client k8sclient.Reader, namespace
return "", nil
}
+
+// GetTolerations build an array of Tolerations from an array of string
+func GetTolerations(taints []string) ([]corev1.Toleration, error) {
Review comment:
What I could propose is:
- Move all the _client_ methods, that use `context context.Context, client k8sclient.Reader`, in a `client.go` or `reader.go` file
- Move the `GetTolerations` into either a `factory.go` file, if we plan to add more factory methods or group the existing ones, or a `toleration.go` file, if we want to be more granular.
I understand this is opinion when it comes to naming and grouping, so I'll trust and respect your choice :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on a change in pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
astefanutti commented on a change in pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#discussion_r593107116
##########
File path: pkg/util/kubernetes/util.go
##########
@@ -228,3 +232,37 @@ func ResolveValueSource(ctx context.Context, client k8sclient.Reader, namespace
return "", nil
}
+
+// GetTolerations build an array of Tolerations from an array of string
+func GetTolerations(taints []string) ([]corev1.Toleration, error) {
Review comment:
Yes and no 😉.
I understand it already contains client methods for other resources, and few other miscellaneous
methods like: `ToJSON`, `ToYAML` `ResolveValueSource` `GetSecretRefValue`. I'd argue that the main difference is that `GetTolerations(taints []string)` is a factory method, while the other, resources related methods, are actually client getters. So yes, because it already breaks the single responsibility principle and it's not a big deal :) and no because this is not a good reason to introduce another kind of method to it 😄.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] heiko-braun commented on pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
heiko-braun commented on pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#issuecomment-796843007
Is this an install time setting or can it be changed after the fact?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on a change in pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
squakez commented on a change in pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#discussion_r593180377
##########
File path: pkg/util/kubernetes/util.go
##########
@@ -228,3 +232,37 @@ func ResolveValueSource(ctx context.Context, client k8sclient.Reader, namespace
return "", nil
}
+
+// GetTolerations build an array of Tolerations from an array of string
+func GetTolerations(taints []string) ([]corev1.Toleration, error) {
Review comment:
Haha, yeah, those are valid points too. With 8118aae I tried to refactor in order to have a tidier organization. I am not entirely sure about the names though. What do you think @astefanutti?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on a change in pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
squakez commented on a change in pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#discussion_r593099695
##########
File path: pkg/util/kubernetes/util.go
##########
@@ -228,3 +232,37 @@ func ResolveValueSource(ctx context.Context, client k8sclient.Reader, namespace
return "", nil
}
+
+// GetTolerations build an array of Tolerations from an array of string
+func GetTolerations(taints []string) ([]corev1.Toleration, error) {
Review comment:
I think it is appropriate keeping as it is. I'll explain why. You can see that such utility unit is bundling different methods treating `corev1` structs. I tried to be as much cohesive as possible. If I use another utility unit I feel I break that cohesion just for the purpose of reducing the size. Finally I preferred to have better cohesion versus the lenght of the `util`. Did I convince you?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
squakez commented on pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#issuecomment-796847416
> Is this an install time setting or can it be changed after the fact?
It will influence the operator `Deployment` spec adding a list of `Toleration`s. Once installed, if you want to change that you will need to do update the `Deployment` spec, probably manually, and reschedule the operator pods.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
astefanutti commented on pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#issuecomment-808204844
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
squakez commented on pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#issuecomment-797390793
> @squakez what would you think of applying the others customizers from the operator Deployment, like the monitoring port, to the CSV as well? I think you've almost done all the work with that PR, and it would avoid having some asymmetric options.
Yeah, I have in mind to add `NodeSelector` as well. I can create an issue to keep track of all of them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti merged pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
astefanutti merged pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on a change in pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
astefanutti commented on a change in pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#discussion_r593086326
##########
File path: pkg/util/kubernetes/util.go
##########
@@ -228,3 +232,37 @@ func ResolveValueSource(ctx context.Context, client k8sclient.Reader, namespace
return "", nil
}
+
+// GetTolerations build an array of Tolerations from an array of string
+func GetTolerations(taints []string) ([]corev1.Toleration, error) {
Review comment:
May I suggest to move it into a dedicated file? The `util.go` has grown too big IMHO.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] heiko-braun commented on pull request #2114: feat(operator): toleration install flag
Posted by GitBox <gi...@apache.org>.
heiko-braun commented on pull request #2114:
URL: https://github.com/apache/camel-k/pull/2114#issuecomment-796880646
Thanks for the clarification @squakez
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org