Refactor servicecatalog provisioned product resource #3
Conversation
…amount of parameters
|
It is a draft version, when it will pass pre review I will add changes to setup.go |
| @@ -192,3 +155,25 @@ func (c *custom) preDelete(_ context.Context, cr *svcapitypes.ProvisionedProduct | |||
| obj.ProvisionedProductName = aws.String(meta.GetExternalName(cr)) | |||
| return false, nil | |||
| } | |||
|
|
|||
| func setConditions(describeRecordOutput *svcsdk.DescribeRecordOutput, resp *svcsdk.DescribeProvisionedProductOutput, cr *svcapitypes.ProvisionedProduct) { | |||
There was a problem hiding this comment.
This func still can't pass the linter, I will fix it tomorrow
There was a problem hiding this comment.
The function looks ok, it is well defined and, yes, it has many cases, but it is consistent in what it does.
So I suggest to not break it or condensate too much the code just for the sake of cyclomatic complexity (it is 11 ...) because the readability will be worst in the end.
| ) | ||
|
|
||
| func provisioningParamsAreChanged(cfStackParams []cfsdkv2types.Parameter, currentParams []*svcapitypes.ProvisioningParameter) bool { | ||
| if len(cfStackParams) != len(currentParams) { |
There was a problem hiding this comment.
This check will be produce false positive isUpToDate check If we okay with that it is not necessary to specify every parameter
There was a problem hiding this comment.
The problem is that idk how to detect removed parameters. Providers doesn't contain info about previous params, ofc we can cache them but it's not persistant.
Thats why ExistingParameterHasBeenRemoved unit tests fails for now
| @@ -105,23 +114,158 @@ func TestIsUpToDate(t *testing.T) { | |||
| args | |||
| want | |||
| }{ | |||
| "ArtifactIdHasChanged": { | |||
There was a problem hiding this comment.
I added tests for all major casses with Artifact and Product params
| @@ -192,3 +155,25 @@ func (c *custom) preDelete(_ context.Context, cr *svcapitypes.ProvisionedProduct | |||
| obj.ProvisionedProductName = aws.String(meta.GetExternalName(cr)) | |||
| return false, nil | |||
| } | |||
|
|
|||
| func setConditions(describeRecordOutput *svcsdk.DescribeRecordOutput, resp *svcsdk.DescribeProvisionedProductOutput, cr *svcapitypes.ProvisionedProduct) { | |||
There was a problem hiding this comment.
The function looks ok, it is well defined and, yes, it has many cases, but it is consistent in what it does.
So I suggest to not break it or condensate too much the code just for the sake of cyclomatic complexity (it is 11 ...) because the readability will be worst in the end.
| } | ||
| func (c *custom) getArtifactID(ds *svcapitypes.ProvisionedProductParameters) (string, error) { | ||
| input := svcsdk.DescribeProductInput{ | ||
| Id: ds.ProductID, |
There was a problem hiding this comment.
can both ds.ProductID and ds.ProductName be nil?
…Status.AtProvider.LastProvisioningParameters
| } | ||
| func (c *custom) getArtifactID(ds *svcapitypes.ProvisionedProductParameters) (string, error) { | ||
| input := svcsdk.DescribeProductInput{ | ||
| Id: ds.ProductID, |
| return nil | ||
| } | ||
|
|
||
| func (c *custom) postCreate(_ context.Context, cr *svcapitypes.ProvisionedProduct, obj *svcsdk.ProvisionProductOutput, cre managed.ExternalCreation, err error) (managed.ExternalCreation, error) { |
There was a problem hiding this comment.
This func will be redundant if we remove spec.forProvider.name
| return obs, nil | ||
| } | ||
|
|
||
| func preCreate(_ context.Context, cr *svcapitypes.ProvisionedProduct, obj *svcsdk.ProvisionProductInput) error { |
There was a problem hiding this comment.
This func will be redundant if we remove spec.forProvider.name
| } | ||
| meta.SetExternalName(cr, *cr.Spec.ForProvider.Name) |
There was a problem hiding this comment.
This line will be redundant if we remove spec.forProvider.name
| @@ -77,6 +77,10 @@ resources: | |||
| from: | |||
| operation: DescribeProvisionedProduct | |||
| path: ProvisionedProductDetail.ProvisioningArtifactId | |||
| LastProvisioningParameters: | |||
| is_read_only: true | |||
| custom_field: | |||
There was a problem hiding this comment.
Actually it's not necessary to expose all params as is, for comparison is enough to know the length of array.
But idk how to generate custom field with type int in the status.
"type: int" works only if "is_read_only" is false
…tus.atProvider.ProvisioningParameters.LastProvisioningParameters(previous ones)
|
The new development branch contains the changes from branch "refactor-servicecatalog-provisioned-product-resource" |
teeverr
deleted the
refactor-servicecatalog-provisioned-product-resource
branch
Description of your changes
Fixes #
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested