feat: generate CRD & mapper for DataFlowFlexTemplateJob #2557
Conversation
justinsb
force-pushed
the
generate_dataflowflextemplatejob
branch
from
8c3c7d4
to
424e6a3
Compare
| KmsKeyNameRef *refs.KMSCryptoKeyRef `json:"kmsKeyNameRef,omitempty"` | ||
|
|
||
| // Configuration for VM IPs. | ||
| IpConfiguration *string `json:"ipConfiguration,omitempty"` |
There was a problem hiding this comment.
Good call. I think it wasn't caught by the apichecker because that checks the JSON form (via the CRD)
| // Docker registry location of container image to use for the 'worker harness. | ||
| // Default is the container for the version of the SDK. Note this field is | ||
| // only valid for portable pipelines. | ||
| SdkContainerImage *string `json:"sdkContainerImage,omitempty"` |
There was a problem hiding this comment.
nit: shall this be SDKContainerImage?
pkg/controller/direct/context.go
Outdated
| Object *unstructured.Unstructured | ||
| } | ||
|
|
||
| func (c *Context) StatusLROStarted() error { |
There was a problem hiding this comment.
This seems to be not used. Can you add a comment for future work in case we miss it?
There was a problem hiding this comment.
Oops, I added it by mistake. I'll remove.
apis/refs/v1beta1/projectref.go
Outdated
| if projectID := src.GetAnnotations()["cnrm.cloud.google.com/project-id"]; projectID != "" { | ||
| return &Project{ProjectID: projectID}, nil | ||
| } |
There was a problem hiding this comment.
Personally I'd prefer having this project-id annotation being handled in each individual KCC resource because this is more of a backward compatible feature and is not what we recommend developers to use (we recommend spec.projectRef). WDYT?
There was a problem hiding this comment.
So the challenge is that it's pretty easy to forget, and we might not have test coverage to catch it!
Have we (aka you :-) ) written the "don't use project-id" guidance?
Personally I think we should continue to support the "fallback to namespace name" functionality (which is what is currently implemented by the project-id annotation, though we could do it another way). Where we have included projectRef, we have made it mandatory. I actually disagree with that, because it breaks users' ability to have the k8s namespace == the project-id and not have to specify projects everywhere.
But I will move this so that we can reach a full decision / direction here, rather than sneaking it in an unrelated PR :-)
cc @cheftako
There was a problem hiding this comment.
"k8s namespace == the project-id" is nit. My concerns is that KCC have already built several conventions but does not give a complete story and users cannot purely based on the KCC conventions to guess out the correct answers. i.e. If using project-id, users cannot use it as a no-brainer, they need to check if each resource supports that annotation or not. For spec.projectRef, at least users can rely on the CRD validation to make some reasonable guess and self-correct.
| @@ -27,7 +27,7 @@ import ( | |||
| ) | |||
|
|
|||
| type IAMServiceAccountRef struct { | |||
| /* The GCP Service Account Email used for auth when secretType is gcpServiceAccount. Allowed value: The `email` field of an `IAMServiceAccount` resource. */ | |||
| /* The `email` field of an `IAMServiceAccount` resource. */ | |||
There was a problem hiding this comment.
(Is this from some other PRs?)
I think we want to build a convention for resource reference: that the <Kind>Ref.external is the GCP asset inventory-like value. So I'd vote against accepting email as the external value.
If it is for the backward compatibility, we can use IAMServiceAccountRef.email instead (warning that the external is deprecating in the status)? Not a blocker of this PR.
There was a problem hiding this comment.
So I removed the "The GCP Service Account Email used for auth when secretType is gcpServiceAccount. " prefix. I think that's specific to one API (though I'm not sure which one). But "The GCP Service Account Email used for auth when secretType is gcpServiceAccount. " should be on the Ref, not on external...
The problem/feature is that fixing it once here means it is fixed in all the APIs that use IAMServiceAccountRef
There was a problem hiding this comment.
Totally agree.
But still my concern is about the convention that we are using "external" to represent something other than the asset inventory format (email) . But as long as the IAMSARef is aligned among all its references, I'm okay with the format. (IAM is special in many ways anyways, so the convention diff is not a big thing)
|
/approve Code looks good (unfortunately the presubmit fails) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma 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:
Approvers can indicate their approval by writing |
|
Issue is #2571 |
justinsb
force-pushed
the
generate_dataflowflextemplatejob
branch
from
424e6a3
to
9d0f76e
Compare
justinsb
force-pushed
the
generate_dataflowflextemplatejob
branch
3 times, most recently
from
21d337c
to
9ecdf32
Compare
|
I split out the controller logic, that should probably be a separate PR (even though it was helpful to write it at the same time!) |
justinsb
force-pushed
the
generate_dataflowflextemplatejob
branch
from
9ecdf32
to
2b0bd08
Compare
justinsb
force-pushed
the
generate_dataflowflextemplatejob
branch
from
2b0bd08
to
6c4ab83
Compare
google-oss-prow
bot
merged commit 1500666
into
GoogleCloudPlatform:master
No description provided.