Skip to content

Commit

Permalink
Support suffix exception and add tests
Browse files Browse the repository at this point in the history
  • Loading branch information
ryansteakley committed Jun 18, 2021
1 parent 32c9216 commit 318990d
Show file tree
Hide file tree
Showing 11 changed files with 119 additions and 16 deletions.
4 changes: 2 additions & 2 deletions pkg/generate/ack/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ var (
"ResourceExceptionCode": func(r *ackmodel.CRD, httpStatusCode int) string {
return r.ExceptionCode(httpStatusCode)
},
"GoCodeSetExceptionMessagePrefixCheck": func(r *ackmodel.CRD, httpStatusCode int) string {
return code.CheckExceptionMessagePrefix(r.Config(), r, httpStatusCode)
"GoCodeSetExceptionMessageCheck": func(r *ackmodel.CRD, httpStatusCode int) string {
return code.CheckExceptionMessage(r.Config(), r, httpStatusCode)
},
"GoCodeSetReadOneOutput": func(r *ackmodel.CRD, sourceVarName string, targetVarName string, indentLevel int, performSpecUpdate bool) string {
return code.SetResource(r.Config(), r, ackmodel.OpTypeGet, sourceVarName, targetVarName, indentLevel, performSpecUpdate)
Expand Down
18 changes: 13 additions & 5 deletions pkg/generate/code/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,27 +24,35 @@ import (
"github.com/aws-controllers-k8s/code-generator/pkg/names"
)

// CheckExceptionMessagePrefix returns Go code that contains a condition to
// check if the message_prefix specified for a particular HTTP status code in
// CheckExceptionMessage returns Go code that contains a condition to
// check if the message_prefix/message_suffix specified for a particular HTTP status code in
// generator config is a prefix for the exception message returned by AWS API.
// If message_prefix field was not specified for this HTTP code in generator
// If message_prefix/message_suffix field was not specified for this HTTP code in generator
// config, we return an empty string
//
// Sample Output:
//
// && strings.HasPrefix(awsErr.Message(), "Could not find model")
func CheckExceptionMessagePrefix(
// && strings.HasSuffix(awsErr.Message(), "does not exist.")
func CheckExceptionMessage(
cfg *ackgenconfig.Config,
r *model.CRD,
httpStatusCode int,
) string {
rConfig, ok := cfg.ResourceConfig(r.Names.Original)
if ok && rConfig.Exceptions != nil {
excConfig, ok := rConfig.Exceptions.Errors[httpStatusCode]
if ok && excConfig.MessagePrefix != nil {
if !ok {
return ""
}
if excConfig.MessagePrefix != nil {
return fmt.Sprintf("&& strings.HasPrefix(awsErr.Message(), \"%s\") ",
*excConfig.MessagePrefix)
}
if excConfig.MessageSuffix != nil {
return fmt.Sprintf("&& strings.HasSuffix(awsErr.Message(), \"%s\") ",
*excConfig.MessageSuffix)
}
}
return ""
}
Expand Down
9 changes: 9 additions & 0 deletions pkg/generate/config/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,15 @@ type ErrorConfig struct {
// later is a terminal state.
// In Go SDK terms - awsErr.Message()
MessagePrefix *string `json:"message_prefix,omitempty"`
// MessageSuffix is an optional string field to be checked as suffix of the
// exception message in addition to exception name. This is needed for HTTP codes
// where the exception name alone is not sufficient to determine the type of error.
// Example: SageMaker service throws ValidationException if job does not exist
// as well as if IAM role does not have sufficient permission to fetch the dataset
// For the former controller should proceed with creation of job whereas the
// later is a terminal state.
// In Go SDK terms - awsErr.Message()
MessageSuffix *string `json:"message_suffix,omitempty"`
}

// RenamesConfig contains instructions to the code generator how to rename
Expand Down
4 changes: 2 additions & 2 deletions pkg/generate/crossplane/crossplane.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ var (
"ResourceExceptionCode": func(r *ackmodel.CRD, httpStatusCode int) string {
return r.ExceptionCode(httpStatusCode)
},
"GoCodeSetExceptionMessagePrefixCheck": func(r *ackmodel.CRD, httpStatusCode int) string {
return code.CheckExceptionMessagePrefix(r.Config(), r, httpStatusCode)
"GoCodeSetExceptionMessageCheck": func(r *ackmodel.CRD, httpStatusCode int) string {
return code.CheckExceptionMessage(r.Config(), r, httpStatusCode)
},
"GoCodeSetReadOneOutput": func(r *ackmodel.CRD, sourceVarName string, targetVarName string, indentLevel int, performSpecUpdate bool) string {
return code.SetResource(r.Config(), r, ackmodel.OpTypeGet, sourceVarName, targetVarName, indentLevel, performSpecUpdate)
Expand Down
74 changes: 74 additions & 0 deletions pkg/generate/sagemaker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/aws-controllers-k8s/code-generator/pkg/generate/code"
"github.com/aws-controllers-k8s/code-generator/pkg/testutil"
)

Expand Down Expand Up @@ -66,3 +67,76 @@ func TestSageMaker_ARN_Field_Override(t *testing.T) {
assert.Equal(true, crd.IsPrimaryARNField("JobDefinitionArn"))

}

func TestSageMaker_Error_Prefix_Message(t *testing.T) {
assert := assert.New(t)
require := require.New(t)

g := testutil.NewGeneratorForService(t, "sagemaker")

crds, err := g.GetCRDs()
require.Nil(err)

crd := getCRDByName("TrainingJob", crds)
require.NotNil(crd)

require.NotNil(crd.Ops)

assert.NotNil(crd.Ops.ReadOne)

// "DescribeTrainingJob":{
// "name":"DescribeTrainingJob",
// "http":{
// "method":"POST",
// "requestUri":"/"
// },
// "input":{"shape":"DescribeTrainingJobRequest"},
// "output":{"shape":"DescribeTrainingJobResponse"},
// "errors":[
// {"shape":"ResourceNotFound"}
// ]
// },

// Which does not indicate that the error is a 404 :( So, the logic in the
// CRD.ExceptionCode(404) method needs to get its override from the
// generate.yaml configuration file.
assert.Equal("ValidationException", crd.ExceptionCode(404))

// Validation Exception has prefix Requested resource not found.
assert.Equal("&& strings.HasPrefix(awsErr.Message(), \"Requested resource not found\") ", code.CheckExceptionMessage(crd.Config(), crd, 404))
}

func TestSageMaker_Error_Suffix_Message(t *testing.T) {
assert := assert.New(t)
require := require.New(t)

g := testutil.NewGeneratorForService(t, "sagemaker")

crds, err := g.GetCRDs()
require.Nil(err)

crd := getCRDByName("ModelPackageGroup", crds)
require.NotNil(crd)

require.NotNil(crd.Ops)
assert.NotNil(crd.Ops.ReadOne)

// "DescribeModelPackageGroup":{
// "name":"DescribeModelPackageGroup",
// "http":{
// "method":"POST",
// "requestUri":"/"
// },
// "input":{"shape":"DescribeModelPackageGroupInput"},
// "output":{"shape":"DescribeModelPackageGroupOutput"}
// }

// Does not list an error however a ValidationException can occur
// Which does not indicate that the error is a 404 :( So, the logic in the
// CRD.ExceptionCode(404) method needs to get its override from the
// generate.yaml configuration file.
assert.Equal("ValidationException", crd.ExceptionCode(404))

// Validation Exception has suffix ModelPackageGroup arn:aws:sagemaker:/ does not exist
assert.Equal("&& strings.HasSuffix(awsErr.Message(), \"does not exist.\") ", code.CheckExceptionMessage(crd.Config(), crd, 404))
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,18 @@ resources:
fields:
JobDefinitionArn:
is_arn: true
TrainingJob:
exceptions:
errors:
404:
code: ValidationException
message_prefix: Requested resource not found
ModelPackageGroup:
exceptions:
errors:
404:
code: ValidationException
message_suffix: does not exist.
ignore:
resource_names:
- Algorithm
Expand Down Expand Up @@ -36,7 +48,7 @@ ignore:
- ModelBiasJobDefinition
- ModelExplainabilityJobDefinition
- ModelPackage
- ModelPackageGroup
# ModelPackageGroup
- ModelQualityJobDefinition
- MonitoringSchedule
- NotebookInstanceLifecycleConfig
Expand All @@ -46,7 +58,7 @@ ignore:
- PresignedNotebookInstanceUrl
- ProcessingJob
- Project
- TrainingJob
# TrainingJob
- TransformJob
- TrialComponent
- Trial
Expand Down
2 changes: 1 addition & 1 deletion templates/crossplane/pkg/conversions.go.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,5 @@ func Generate{{ .CRD.Ops.Delete.InputRef.Shape.ShapeName }}(cr *svcapitypes.{{ .
// IsNotFound returns whether the given error is of type NotFound or not.
func IsNotFound(err error) bool {
awsErr, ok := err.(awserr.Error)
return ok && awsErr.Code() == "{{ ResourceExceptionCode .CRD 404 }}" {{ GoCodeSetExceptionMessagePrefixCheck .CRD 404 }}
return ok && awsErr.Code() == "{{ ResourceExceptionCode .CRD 404 }}" {{ GoCodeSetExceptionMessageCheck .CRD 404 }}
}
2 changes: 1 addition & 1 deletion templates/pkg/resource/sdk_find_get_attributes.go.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func (rm *resourceManager) sdkFind(
{{- end }}
rm.metrics.RecordAPICall("GET_ATTRIBUTES", "{{ .CRD.Ops.GetAttributes.ExportedName }}", respErr)
if respErr != nil {
if awsErr, ok := ackerr.AWSError(respErr); ok && awsErr.Code() == "{{ ResourceExceptionCode .CRD 404 }}" {{ GoCodeSetExceptionMessagePrefixCheck .CRD 404 }}{
if awsErr, ok := ackerr.AWSError(respErr); ok && awsErr.Code() == "{{ ResourceExceptionCode .CRD 404 }}" {{ GoCodeSetExceptionMessageCheck .CRD 404 }}{
return nil, ackerr.NotFound
}
return nil, respErr
Expand Down
2 changes: 1 addition & 1 deletion templates/pkg/resource/sdk_find_read_many.go.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func (rm *resourceManager) sdkFind(
{{- end }}
rm.metrics.RecordAPICall("READ_MANY", "{{ .CRD.Ops.ReadMany.ExportedName }}", respErr)
if respErr != nil {
if awsErr, ok := ackerr.AWSError(respErr); ok && awsErr.Code() == "{{ ResourceExceptionCode .CRD 404 }}" {{ GoCodeSetExceptionMessagePrefixCheck .CRD 404 }}{
if awsErr, ok := ackerr.AWSError(respErr); ok && awsErr.Code() == "{{ ResourceExceptionCode .CRD 404 }}" {{ GoCodeSetExceptionMessageCheck .CRD 404 }}{
return nil, ackerr.NotFound
}
return nil, respErr
Expand Down
2 changes: 1 addition & 1 deletion templates/pkg/resource/sdk_find_read_one.go.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func (rm *resourceManager) sdkFind(
{{- end }}
rm.metrics.RecordAPICall("READ_ONE", "{{ .CRD.Ops.ReadOne.ExportedName }}", respErr)
if respErr != nil {
if awsErr, ok := ackerr.AWSError(respErr); ok && awsErr.Code() == "{{ ResourceExceptionCode .CRD 404 }}" {{ GoCodeSetExceptionMessagePrefixCheck .CRD 404 }}{
if awsErr, ok := ackerr.AWSError(respErr); ok && awsErr.Code() == "{{ ResourceExceptionCode .CRD 404 }}" {{ GoCodeSetExceptionMessageCheck .CRD 404 }}{
return nil, ackerr.NotFound
}
return nil, respErr
Expand Down
2 changes: 1 addition & 1 deletion templates/pkg/resource/sdk_update_set_attributes.go.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func (rm *resourceManager) sdkUpdate(
_, respErr := rm.sdkapi.{{ .CRD.Ops.SetAttributes.ExportedName }}WithContext(ctx, input)
rm.metrics.RecordAPICall("SET_ATTRIBUTES", "{{ .CRD.Ops.SetAttributes.ExportedName }}", respErr)
if respErr != nil {
if awsErr, ok := ackerr.AWSError(respErr); ok && awsErr.Code() == "{{ ResourceExceptionCode .CRD 404 }}" {{ GoCodeSetExceptionMessagePrefixCheck .CRD 404 }}{
if awsErr, ok := ackerr.AWSError(respErr); ok && awsErr.Code() == "{{ ResourceExceptionCode .CRD 404 }}" {{ GoCodeSetExceptionMessageCheck .CRD 404 }}{
// Technically, this means someone deleted the backend resource in
// between the time we got a result back from sdkFind() and here...
return nil, ackerr.NotFound
Expand Down

0 comments on commit 318990d

Please sign in to comment.