feat(mexp): Onboarding Modelexperiments - #1547
Conversation
|
This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it. |
|
After those comments are resolved I will start to run the acceptance tests and manual testing |
| resource "time_rotating" "rotate" { | ||
| rotation_days = 80 | ||
| } | ||
|
|
||
| resource "stackit_modelexperiments_token" "token" { | ||
| project_id = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" | ||
| name = "Example token nane" | ||
| region = "eu01" | ||
| instance_id = stackit_modelexperiments_instance.example.instance_id | ||
| description = "Example token description" | ||
| ttl_duration = "1h" |
There was a problem hiding this comment.
If I got it correctly, the ttl_duration value defines when the token expires. In this case it would be after one hour, but the time_rotating resource would trigger a recreate after 80 days. I think it would be nice, if this examples contains values which fit better together
| updateInstanceResp, err := i.client.PartialUpdateInstance(ctx, projectId, region, instanceId).PartialUpdateInstancePayload(*payload).Execute() | ||
| if err != nil { | ||
| core.LogAndAddError(ctx, &resp.Diagnostics, "Error updating AI Model Experiments instance", fmt.Sprintf("Calling API: %v", err)) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
is the update synchronous? Because based on the API spec, it returns a 202 which means the update is accepted but not done. There is also an updating state. But I couldn't find a waiter for the update
| if instance.Id == "" { | ||
| return fmt.Errorf("instance id not present") | ||
| } |
There was a problem hiding this comment.
check if instance == nil is missing. Potential nil pointer exception
| type InstanceDataSourceModel struct { | ||
| Model | ||
| } |
There was a problem hiding this comment.
Is there a reason why there is a separate datasource model, which just has the resource model embedded? If not, then the Model can be just directly used.
|
|
||
| getInstanceTokenResp, err := i.client.GetInstanceToken(ctx, projectId, region, tokenId, instanceId).Execute() | ||
| if err != nil { | ||
| core.LogAndAddError(ctx, &resp.Diagnostics, "Error reading AI Model Experiments instance token", fmt.Sprintf("Calling API: %v", err)) |
There was a problem hiding this comment.
We have a utils.LogError() function, which should be used in datasources for error handling and providing a more descriptive error message.
See example usage:
| return fmt.Errorf("model input is nil") | ||
| } | ||
|
|
||
| if token.Id == "" { |
There was a problem hiding this comment.
nil check for token is missing
| return fmt.Errorf("model input is nil") | ||
| } | ||
|
|
||
| if token.Id == "" { |
There was a problem hiding this comment.
potential nil pointer on token
| return fmt.Errorf("model input is nil") | ||
| } | ||
|
|
||
| if token.Id == "" { |
There was a problem hiding this comment.
potential nil pointer on token
| ConfigVariables: testModelexperimentsInstanceConfigVarsMinUpdated(), | ||
| Check: resource.ComposeAggregateTestCheckFunc( |
There was a problem hiding this comment.
Please add here some config plan checks, to ensure that only a update and not a replace will be triggered. Similar like here:
| ConfigVariables: testModelexperimentsInstanceConfigVarsMaxUpdated(), | ||
| Check: resource.ComposeAggregateTestCheckFunc( |
There was a problem hiding this comment.
same here
3 participants
Description
This is the onboarding PR for integration STACKIT Modelexperiments into the STACKIT Terraform Provider.
It adds for customer the ability to:
Checklist
make fmtexamples/directory)make generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)