move global environmentd resources to a separate crd#34576
move global environmentd resources to a separate crd#34576doy-materialize wants to merge 2 commits intomainfrom
Conversation
doy-materialize
force-pushed
the
push-rulnvrvwszuw
branch
from
7e5b88e to
6ed33e0
Compare
doy-materialize
force-pushed
the
push-rulnvrvwszuw
branch
from
6ed33e0 to
a69bd34
Compare
doy-materialize
requested review from
alex-hunt-materialize
and removed request for
jubrad
| namespaced, | ||
| group = "materialize.cloud", | ||
| version = "v1alpha1", | ||
| kind = "Environment", |
There was a problem hiding this comment.
It worries me a bit to reuse the name Environment here. We previously had an Environment type that had a completely different purpose. This is potentially going to get people confused internally, and it may conflict with any left over CRDs still in staging and production environments.
There was a problem hiding this comment.
i'm not super concerned about things breaking - this is something we can pretty easily clean up by hand if necessary. do you have a suggestion for a better name?
There was a problem hiding this comment.
Maybe something like MaterializeMultiGenerationResources? I'd really like to make it clear that this is the stuff that applies to multiple generations. Environment is super generic, and doesn't really make that clear.
| /// If running in AWS, override the IAM role to use to give | ||
| /// environmentd access to the persist S3 bucket. | ||
| #[kube(deprecated)] | ||
| pub environmentd_iam_role_arn: Option<String>, |
There was a problem hiding this comment.
Should we just not have this field? If this field is set in the Materialize CR, we could just set "eks.amazonaws.com/role-arn" in the service account annotations instead.
I guess it doesn't really matter too much, since users shouldn't be messing with this CRD directly, but it would be one less thing we have to change later.
There was a problem hiding this comment.
yeah, i wasn't sure what to do here - in order for this to be a drop-in replacement, we need to include it here, but you're probably right that we should just fix this on the cloud side first before we merge this and then we can leave it out.
|
thinking about this some more, i think #34806 is actually a better idea here - let me know what you think |
|
Yeah, I prefer #34806 I think we may eventually want to change these objects to be generation aware, but that's a much larger undertaking. |
2 participants
Motivation
continuing to refactor
Tips for reviewer
this one is a lot simpler than the previous few
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.