apis: ApigeeEnvgroupAttachment #3605
Conversation
Signed-off-by: Alex Pana <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Alex Pana <[email protected]>
| if obj.Spec.Envgroup == "" { | ||
| return nil, fmt.Errorf("spec.envgroup cannot be empty") | ||
| } | ||
| if obj.Spec.Environment == "" { |
There was a problem hiding this comment.
I don't think it is necessary to validate this here? Because spec.environment is not part of the identity
| // holds the GCP identifier for the KRM object. | ||
| type ApigeeEnvgroupAttachmentRef struct { | ||
| // A reference to an externally managed ApigeeEnvgroupAttachment resource. | ||
| // Should be in the format "projects/*/locations/global/envgroups/*/attachments/*". |
There was a problem hiding this comment.
This format looks incorrect to me, shouldn't it be in the pattern of organizations/{{organizationID}}/envgroups/{{envgroupID}}/attachments/{{attachmentID}} ?
| func ParseApigeeEnvgroupAttachmentExternalRef(external string) (parent *EnvgroupAttachmentParent, resourceID string, err error) { | ||
| tokens := strings.Split(external, "/") | ||
| if len(tokens) != 6 { | ||
| return nil, "", fmt.Errorf("invalid external format: %s, expecting organizations/*/envgroups/*/attachments/*", external) |
There was a problem hiding this comment.
For consistency with #3398, I think we should use the same pattern (using {{xyzID}} instead of *)
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if actualParent.Envgroup != obj.Spec.Envgroup { |
There was a problem hiding this comment.
tiny nit, feels a bit weird to compare these fields out-of-order. i.e. envgroup -> org -> id instead of org -> envgroup -> id
| // +required | ||
| OrganizationRef *refv1alpha1.ApigeeOrganizationRef `json:"organizationRef"` | ||
|
|
||
| // Immutable. The Apigee environment to attach to. |
There was a problem hiding this comment.
I thought this resource attaches an environment to an envgroup. Not the other way around. Where did this comment come from? Was it originally from the API? In any case, I think we should update this.
| // Immutable. The Apigee environment to attach to. | ||
| Environment string `json:"environment"` | ||
|
|
||
| // Immutable. The Apigee environment group which will host the environment. |
There was a problem hiding this comment.
Nit, would put this field before Environment, because Envgroup is part of the parent.
| } | ||
|
|
||
| // ApigeeEnvgroupAttachmentStatus defines the observed state of ApigeeEnvgroupAttachment | ||
| type ApigeeEnvgroupAttachmentStatus struct { |
There was a problem hiding this comment.
There should be an observedState.createdAt field?
| // holds the GCP identifier for the KRM object. | ||
| type ApigeeEnvironmentRef struct { | ||
| // A reference to an externally managed ApigeeEnvironment resource. | ||
| // Should be in the format "projects/*/environments/*". |
There was a problem hiding this comment.
This should have format starting with organizations/{{organizationID}}/..., no?
| return "", fmt.Errorf("reading status.externalRef: %w", err) | ||
| } | ||
| if !found { | ||
| // todo acpana careful to deal with this |
There was a problem hiding this comment.
In case you are not the next person who hits this issue, could you please put a more descriptive comment? Would suggest linking to bug / ticket and including some details around expected issue behavior, resolution hints, etc.
| return r.External, nil | ||
| } | ||
|
|
||
| // todo acpana this should be in a _identity.go file eventually |
| tokens := strings.Split(external, "/") | ||
|
|
||
| if len(tokens) != 4 || tokens[0] != "organizations" || tokens[2] != "environments" { | ||
| return "", "", fmt.Errorf("external should be organizations/{organization}/environments/{environment}, got: %s", external) |
There was a problem hiding this comment.
For consistency with other resources, we should use {{organizationID}} (double curly)
| OrganizationRef *refv1alpha1.ApigeeOrganizationRef `json:"organizationRef"` | ||
|
|
||
| // Immutable. The Apigee environment to attach to. | ||
| Environment string `json:"environment"` |
There was a problem hiding this comment.
We should use a reference type here
| Environment string `json:"environment"` | ||
|
|
||
| // Immutable. The Apigee environment group which will host the environment. | ||
| Envgroup string `json:"envgroup"` |
There was a problem hiding this comment.
Same, this should be a reference type, right?
Adds direct style APIs for the
ApigeeEnvgroupAttachment. This is an alpha resource, still running over the TF controller.Next PR: