feat: add t8s kaas offering#1244
Conversation
|
I hope I implemented this correctly, but at least my preliminary tests were successful. |
|
Thanks @cwrau. At first glance, it looks promising! Will have to read carefully and write up a review, but due to vacation, it could take until July 22nd or so... |
|
Question: you wrote that it's based on Gardener, yet you don't seem to use the Gardener API, but some Helm stuff (I still need to read more carefully). Is Gardener not exposed to your customers? If that's true, we should not mention Gardener. |
Däh, where did you read that? 😅 |
afb3305 to
eff053c
Compare
|
Ehlo, just wanted to ask if I need to adjust anything? |
|
No, I'm afraid you just have to wait
|
There was a problem hiding this comment.
Looking quite good! Thank you for contributing this!
I am missing k8s 1.34, and I have a few remarks, but small fry only.
Personally, I'm not into the excessive typing and unit-testing, where the former makes the code harder to read and the latter increases the codebase for marginal gain (because, in my experience so far with these plugins, they won't be changed once they work, and the changes we do would not be covered by the tests). However, this is mainly my personal opinion, and if you prefer the typing and unit-testing, then so be it.
| kind: t8s | ||
| config: | ||
| kubernetesVersion: '1.35' | ||
| version_patch: 2 |
There was a problem hiding this comment.
Ideally, the latest patch version should be selected automatically. For instance, is there a way to enumerate available patch versions?
There was a problem hiding this comment.
Sadly no, to do that we'd have to look at our TeutonetesClouds status and there are internal settings/infos on that object.
There was a problem hiding this comment.
We may be able to live with this restriction for a while, but in the long run, it won't be fun bumping the patch version by hand every time the testcase version-policy-check is failed...
There was a problem hiding this comment.
Oh, I didn't know the patch version was part of the certification, I assumed only the minor was required.
But the patch version will be removed when our customer portal allows machine access, then we'd have no choice of the patch version (or any version for that matter) in any case. (we'd have update schedules just for SCS that have the old versions 😅)
There was a problem hiding this comment.
For SCS, we would like to be able to select the k8s branch (so major.minor), and the patch version can just be whatever is most recent. From what you're writing it doesn't sound so far off.
| "chartRef": HR_CHART_REF, | ||
| "driftDetection": {"mode": "enabled"}, | ||
| "interval": "1m", | ||
| "values": {**HR_VALUES_FIXED, "version": self.k8s_version}, |
There was a problem hiding this comment.
Minor remark: Since this method only uses this one instance variable, I'm wondering whether it's better turned into a function that takes k8s_version as a parameter.
There was a problem hiding this comment.
This would "break" consistency to the other build* and apply* methods. But I can change it if you want
There was a problem hiding this comment.
You're right. But now that is even more concerning: can we not provision multiple clusters at the same time? At the very least two, one for each branch?
There was a problem hiding this comment.
That wasn't planned, no.
K8s RBAC isn't that flexible, but if we have some form of deterministic order we could allow 3; each with an indexed suffix (-1, -2 and -3)
There was a problem hiding this comment.
We can live with one, I suppose. The CI pipeline has been equipped with a semaphore so only one k8s branch will be tested at a time, so we should be safe.
From a programming standpoint, I think the class doesn't make a lot of sense because many of its methods don't even use self. So these methods could be turned into simple functions or staticmethod or classmethod. However, I would rather suggest we pretend that certain quantities are not actually constants and introduce instance variables for them, even if we only ever instantiate them with the same value. Like you just said, even if these values need to be hard-coded/prepared on the server side, several of them could exist, and then this would make sense.
Signed-off-by: Chris Werner Rau <cwrau@cwrau.info>
Ah, we don't have anyone on 1.34 anymore, so I didn't think about that, is that necessary?
Typing is definitely for my own sanity and maintainability, but if that isn't this repos style I can drop it. Same goes for the tests, if those testable changes aren't really happening then OK 😁 |
|
Like I said, we can keep the typing and the tests. I don't want to impose my style upon contributors, and it's a welcome chance to prove me wrong. 😄 So long as 1.34 is officially supported (this would be until Oct 27, 2026), we would need to have it, but Teutonet could also declare that you don't want to be certified before that date 😉 |
3 participants
No description provided.