update for default resource tags #87
Conversation
vijtrip2
changed the title
There was a problem hiding this comment.
Left a suggestion inline.
I think it would be good to support a few more variables, even if they aren't used in the default. My suggestions would be:
%OWNER_ACCOUNT_ID%- taken fromStatus%K8S_RESOURCE_NAME%- taken frommetadata.name%K8S_CRD_VERSION%- the CRD version (v1alpha1)
| case "%CONTROLLER_VERSION%": | ||
| expandedValue = generateControllerVersion(md) |
There was a problem hiding this comment.
Could we split this into two different variables? Perhaps %CONTROLLER_SERVICE% and %CONTROLLER_VERSION%? This way users can, with more granularity, choose their tag formats.
There was a problem hiding this comment.
how about i tackle this in a followup PR?
This PR can deal with refactor of serviceController and addition of a new tag field. Keeping existing expandValue method as is.
Next PR will deal with supporting more reserved keywords, and find + replace keywords in all resource tags, not just the ones that match the switch case.
There was a problem hiding this comment.
The change I suggested will not be backward compatible. If we delay that change but push out this functionality, there is a chance that one of our GA controllers will be released between now and then with breaking compatibility. I'd prefer that if we are going to start enabling this feature, that we start off with the format we want to support in the long-term.
There was a problem hiding this comment.
Yeah, by followup PR, i meant next PR(before releasing this feature).
And I am curious to know what will be the backward incompatible change. I did not get that part.
There was a problem hiding this comment.
Oh the backward incompatibility would be %CONTROLLER_VERSION% changing from s3-v1.0.0 to v1.0.0. But I guess as long as we get both of these out before cutting a new runtime release then I'm fine with it.
There was a problem hiding this comment.
After supporting these different tag variables, I was planning on changing the default tag to services.k8s.aws/controller-version=%CONTROLLER_SERVICE%-%CONTROLLER_VERSION% and then the output tag value will stay the same.
Also I wanted to do inplace substring replacement instead complete string replacement in the next PR.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RedbackThomson, vijtrip2 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue #, if available: aws-controllers-k8s/community#1261 Description of changes: * support '%CONTROLLER_SERVICE%' , '%CONTROLLER_VERSION%', '%K8S_NAMESPACE%' and '%K8S_RESOURCE_NAME%' as the resource tag format * support tag format as substring, and not just complete tag value * As per the [comment](#87 (review)) , I did not add 'OWNER_ACCOUNT_ID' because any AWS related information will be known to customer when describing tags or viewing the resource in AWS console. * I also did not add 'K8S_CRD_VERSION' right now and waiting for multi version support , to find out the best place to find the k8s resource crd version. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue #, if available: aws-controllers-k8s/community#1261
Description of changes:
This PR changes the default ACK resource tags from empty list to []string{"services.k8s.aws/controller-version=%CONTROLLER_VERSION%" ,"services.k8s.aws/namespace=%K8S_NAMESPACE%"}.
Instead of statically passing the controller release version from release artifacts, i made it a dynamic
%CONTROLLER_VERSION%variable. The dynamic variable will allow customers to use this variable in their custom tags as well.I refactored the various service controller metadata fields into a new
ServiceControllerMetadatatype as well.Tested locally that the resource tags get resolved as expected.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.