Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 404: ResourceError handling in sdkFind #421

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

embano1
Copy link
Member

@embano1 embano1 commented Mar 2, 2023

In the Pipes service controller we noticed that the error returned from the API is of type RequestFailure. A workaround is to set code: "" in generator.yaml or for those not aware of this issue fallback using this new type assertion in READ_ONE.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-prow ack-prow bot requested review from a-hilaly and jljaco March 2, 2023 13:08
@@ -34,6 +34,9 @@ func (rm *resourceManager) sdkFind(
{{- end }}
rm.metrics.RecordAPICall("READ_ONE", "{{ .CRD.Ops.ReadOne.ExportedName }}", err)
if err != nil {
if reqErr, ok := ackerr.AWSRequestFailure(err); ok && reqErr.StatusCode() == 404 {
return nil, ackerr.NotFound
+ }
Copy link
Member

@a-hilaly a-hilaly Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+ 👀

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@embano1 I think what @a-hilaly is trying to say here is that the indentation is off here :) gofmt cleans it up in the generated code but for our eyes in reviewing PRs that touch the templates, it's best to replace four spaces with \t characters in the files before saving.

In Vim I do: :%s/ /\t/gI

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i meant + 👁️ , there is a tiny extra character in the last line :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentation is correct, but there was a little + copied from my git patch foo, well spotted @a-hilaly !

Signed-off-by: Michael Gasch <15986659+embano1@users.noreply.github.com>
@ack-prow
Copy link

ack-prow bot commented Mar 2, 2023

@embano1: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ec2-controller-test 5bbec28 link true /test ec2-controller-test

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.

Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's funny that AWSRequestFailure already exists in runtime :)
Looks good to me! We can force merge as the prow errors are not related to this change.
Danke Michael!

@ack-prow ack-prow bot added the approved label Mar 2, 2023
@embano1
Copy link
Member Author

embano1 commented Mar 6, 2023

Can someone please force-merge to get this PR closed :) 🙏🏻

@a-hilaly
Copy link
Member

a-hilaly commented Mar 6, 2023

/cc @RedbackThomson @jljaco

@ack-prow ack-prow bot requested a review from RedbackThomson March 6, 2023 18:03
@RedbackThomson RedbackThomson changed the title Add 404 ResourceError` handling in sdkFind Add 404: ResourceError handling in sdkFind Mar 7, 2023
@RedbackThomson
Copy link
Contributor

/lgtm

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2023
@ack-prow
Copy link

ack-prow bot commented Mar 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, embano1, jljaco, RedbackThomson

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:
  • OWNERS [A-Hilaly,RedbackThomson,jljaco]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@RedbackThomson RedbackThomson merged commit 9373b21 into aws-controllers-k8s:main Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants