add support for domainame #48
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rushmash91 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 |
rushmash91
force-pushed
the
domainame
branch
2 times, most recently
from
e2cb9e4 to
cc46786
Compare
apis/v1alpha1/generator.yaml
Outdated
| path: Status.ID | ||
| is_required: true | ||
| is_immutable: true |
There was a problem hiding this comment.
this looks incorrect..
|
@rushmash91: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Congrats on your first PR @rushmash91 x tamer@ 🚀🚀
left a few comments
| is_required: true |
There was a problem hiding this comment.
not sure if these two are needed..the model should have marked it as required
| - InvalidParameter | ||
| DomainName: |
There was a problem hiding this comment.
Should we rename the resource to DomainNameResource? A resource named DomainName doesn't feel right
There was a problem hiding this comment.
hmm I think Domain name is better for this resource. What about it doesn't feel right?
There was a problem hiding this comment.
It would just be weird to have something like DomainName.Spec.DomainName or DomainName.Spec.Name..it feels very misleading to me..
| DomainName: | ||
| fields: | ||
| DomainName: |
There was a problem hiding this comment.
we could rename this to Name?
| synced: | ||
| when: | ||
| - path: Status.DomainNameStatus |
There was a problem hiding this comment.
maybe rename this to just Status as well?
| "github.com/aws-controllers-k8s/apigateway-controller/pkg/util/patch" | ||
| ) | ||
|
|
||
| func updateDomainNameInput(desired, latest *resource, input *svcsdk.UpdateDomainNameInput, delta *compare.Delta) error { |
There was a problem hiding this comment.
why do we want to return error here?
| defer func() { exit(err) }() | ||
|
|
||
| // Get the ARN of the domain name | ||
| resourceARN := rm.getDomainNameARN(*desired.ko.Spec.DomainName) |
There was a problem hiding this comment.
Create seems to already be returning the ARN https://docs.aws.amazon.com/apigateway/latest/api/API_CreateDomainName.html#apigw-CreateDomainName-response-domainNameArn
Should we instead use latest...ARN?
There was a problem hiding this comment.
maybe we just need to rename it to ARN
Issue aws-controllers-k8s/community#2550
Description of changes:
add support for domainame #tamer
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.