-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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 data capture config to Sagemaker endpoint configuration #11484
Add data capture config to Sagemaker endpoint configuration #11484
Conversation
Just wondering what else needs to be done to get this merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, looks good! some comments on styling and simplifying handling of slices/lists
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
ValidateFunc: validation.StringInSlice([]string{"Input", "Output"}, false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets change this to:
ValidateFunc: validation.StringInSlice(sagemaker.CaptureMode_Values(), false),
} | ||
|
||
cfg := map[string]interface{}{ | ||
//"enable_capture": aws.BoolValue(dataCaptureConfig.EnableCapture), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let remove commented out code 😺
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"csv_content_types": { | ||
Type: schema.TypeList, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can similify this to look like this:
"csv_content_types": {
Type: schema.TypeSet,
MinItems: 1,
MaxItems: 10,
Elem: &schema.Schema{
Type: schema.TypeString,
ValidateFunc: validateSagemakerCsvContentType, },
Optional: true,
ForceNew: true,
},
this will also look more similar to the API as it corresponds to slices CsvContentTypes []*string
ForceNew: true, | ||
}, | ||
|
||
"json_content_types": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
if v, ok := m["csv_content_types"]; ok && len(v.([]interface{})) > 0 { | ||
contentTypes := make([]*string, 0, len(v.([]interface{}))) | ||
|
||
for _, raw := range v.([]interface{}) { | ||
sRaw := raw.(map[string]interface{})["content_type"] | ||
contentTypes = append(contentTypes, aws.String(sRaw.(string))) | ||
} | ||
|
||
c.CsvContentTypes = contentTypes | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the above changes to type are made this can be reduced to:
c.CsvContentTypes = expandStringSet(v.(*Schema.Set))
c.CsvContentTypes = contentTypes | ||
} | ||
|
||
if v, ok := m["json_content_types"]; ok && len(v.([]interface{})) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
if contentTypeHeader.CsvContentTypes != nil { | ||
csvTypes := make([]map[string]interface{}, 0, len(contentTypeHeader.CsvContentTypes)) | ||
|
||
for _, csvType := range contentTypeHeader.CsvContentTypes { | ||
m := map[string]interface{}{ | ||
"content_type": aws.StringValue(csvType), | ||
} | ||
csvTypes = append(csvTypes, m) | ||
} | ||
|
||
l["csv_content_types"] = csvTypes | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again as above, this will reduced to:
l["csv_content_types"] = flattenStringSet(contentTypeHeader.CsvContentTypes)
if contentTypeHeader.JsonContentTypes != nil { | ||
jsonTypes := make([]map[string]interface{}, 0, len(contentTypeHeader.JsonContentTypes)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
Hey @SaiKiranBurle, can you rebase and address comments? I can fork your repo and finish up the needed changes. |
@DrFaust92 please feel free to submit new PR including this commit and we'll get it in. 👍 |
It looks like this was covered by #15887, just that GitHub did not automatically close due to the rebased commit SHA. |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Community Note
Release note for CHANGELOG:
Output from acceptance testing: