search

crossplane-contrib / provider-aws

Crossplane AWS Provider
Apache License 2.0
441 stars 375 forks source link

Tags definition is inconsistent across resources #1244

Closed bobh66 closed 1 year ago

bobh66 commented 2 years ago

What happened?

There are approximately 78 resources that support a "tags" parameter in the spec.forProvider section. Of these, 60 define tags as an array of objects with key and value properties, while the remaining 18 define tags as an object with additionalProperties.type = string.

This makes it impossible to consistently define a set of tags across all resources in a composition since they are different data types.

Given the difference, I'd like to propose that the 18 object types be converted to arrays of objects for consistency.

package/crds/apigatewayv2.aws.crossplane.io_apis.yaml package/crds/apigatewayv2.aws.crossplane.io_domainnames.yaml package/crds/apigatewayv2.aws.crossplane.io_stages.yaml package/crds/apigatewayv2.aws.crossplane.io_vpclinks.yaml package/crds/cloudwatchlogs.aws.crossplane.io_loggroups.yaml package/crds/eks.aws.crossplane.io_addons.yaml package/crds/eks.aws.crossplane.io_clusters.yaml package/crds/eks.aws.crossplane.io_fargateprofiles.yaml package/crds/eks.aws.crossplane.io_identityproviderconfigs.yaml package/crds/eks.aws.crossplane.io_nodegroups.yaml package/crds/glue.aws.crossplane.io_connections.yaml package/crds/glue.aws.crossplane.io_crawlers.yaml package/crds/glue.aws.crossplane.io_jobs.yaml package/crds/kafka.aws.crossplane.io_clusters.yaml package/crds/lambda.aws.crossplane.io_functions.yaml package/crds/mq.aws.crossplane.io_brokers.yaml package/crds/prometheusservice.aws.crossplane.io_workspaces.yaml package/crds/sqs.aws.crossplane.io_queues.yaml

How can we reproduce it?

See Vpc.ec2.aws for an example of tags defined as a list of objects

See LogGroup.cloudwatch.aws for an example of tags defined as an object

What environment did it happen in?

Crossplane version: 1.7

haarchri commented 2 years ago

it depends on the go-sdk version V1/V2 and the resources we using the upstream Implementation without type change

bobh66 commented 2 years ago

Maybe a workaround for this would be to add a MapToList conversion type to the Composition, which would take a map of key value pairs and convert it to a List of maps with {Key: keyname, Value: value} pairs? That would at least allow the user to pass in a single Map of key/value pairs for tags and use it for both types of tag structures.

MisterMX commented 2 years ago

I agree with @bobh66 that this is a really annoying issue. Though I would not use a list, but rather a simple map[string]string because it is independent from sort order and merged instead of overwritten (see #1072 for example).

This is also relevant for #1306.

MisterMX commented 2 years ago

I think it would be comparably easy to change the tagging API for resources that are implemented manually, i.e. ec2.Instance.

For all resources generated with ACK we would have to ignore the tags field in the generator-config.yaml and add the logic by hand.

@haarchri do you think this is something we could go forward with?

tanujd11 commented 2 years ago

@MisterMX anything planned for this issue? This is causing issues in composition. We have to provide tags in two different ways in XR which is bad user experience.

MisterMX commented 2 years ago

No, this is stale at the moment as it is quite complex since it affects almost every resource in the provider.

github-actions[bot] commented 1 year ago

Crossplane does not currently have enough maintainers to address every issue and pull request. This issue has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.