feat: Add code generator for generic resource tagging #1378
Conversation
Signed-off-by: Maximilian Blatt <[email protected]> (external expert on behalf of DB Netz AG)
Add a `common.Tagger` to every resource that has a generated AddTag method. Signed-off-by: Maximilian Blatt <[email protected]> (external expoert on behalf of DB Netz AG)
MisterMX
force-pushed
the
codegen-tagged
branch
4 times, most recently
from
fb1d147 to
5948f4f
Compare
| @@ -16,21 +17,24 @@ See the License for the specific language governing permissions and | |||
| limitations under the License. | |||
| */ | |||
|
|
|||
| // NOTE(negz): See the below link for details on what is happening here. | |||
| // https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module | |||
| // // NOTE(negz): See the below link for details on what is happening here. | |||
There was a problem hiding this comment.
@MisterMX looks like we have double // now ?
| @@ -0,0 +1,18 @@ | |||
| /* | |||
| Copyright 2021 The Crossplane Authors. | |||
There was a problem hiding this comment.
nit: use 2022 for all new files?
| */ | ||
|
|
||
| // Package common contains shared controller logic. | ||
| package common |
There was a problem hiding this comment.
Could we call this something more specific like tagger or tagutil?
https://github.com/golang/go/wiki/CodeReviewComments#package-names
|
looks awesome! I think we could add IsUpToDate helpers the same way later? |
|
|
||
| // AddTag adds a tag to this Key. If it already exists, it will be overwritten. | ||
| // It returns true if the tag has been added/changed. Otherwise false. | ||
| func (mg *Key) AddTag(key string, value string) bool { |
There was a problem hiding this comment.
is this one hand written? If so, add a comment why?
|
@MisterMX did you also try some of the resource types in a real cluster? This is a huge PR, it looks good to me but there might be a "needle in the haystack" here. Is there anything in particular you think we should look at, anything that's not tested or that you feel less confident about? |
|
Will this fix inconsistency in tagging input? (like how most resources expect tags as an array, but some select ones, such as eks Cluster and NodeGroup, expect them to be a map) |
|
@MisterMX can you rebase and Check for latest comments ? Thanks |
|
Crossplane does not currently have enough maintainers to address every issue and pull request. This pull request has been automatically marked as |
|
/fresh |
Description of your changes
This adds a code generator that creates a method
AddTagfor each managed resource that has a fieldSpec.ForProvider.Tagto satisfy the generic interfaceTagged.It enables us to replace each specific
taggerinitializer that is currently copy-pasted for every controller with a central component reducing code redundancy.The generated code abstracts from (most) of the more over a dozen tagging implementations that exist today. The following tag implementations are covered by this generator:
map[string]stringmap[string]*string[]Tag {string, string}[]*Tag {string, string}[]Tag {*string, *string}[]*Tag {*string, *string}[]Tag (with string key and *string value)[]*Tag (with string key and *string value)Note: It does not fix the issue that tags are defined differently for each resource because that is an issue with the underlying ACK generator and AWS SDK (see #1244).
This is a preliminary for #1306.
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
Unit tests