Rename Group to ShortGroup and allow empty short group #144
Conversation
…ys what it does more precisely Signed-off-by: Muvaffak Onus <me@muvaf.com>
…hortGroupName Signed-off-by: Muvaffak Onus <me@muvaf.com>
…e gosec does not accept them Signed-off-by: Muvaffak Onus <me@muvaf.com>
There was a problem hiding this comment.
Thanks @muvaf for the PR. I have consumed these changes in crossplane-contrib/provider-jet-azure#83. Below are some minor comments for your consideration.
| // For example, ShortGroup could be `ec2` where group suffix of the | ||
| // provider is `aws.crossplane.io` and in that case, the full group would | ||
| // be `ec2.aws.crossplane.io` | ||
| ShortGroup string |
There was a problem hiding this comment.
ShortGroup is fine but we can also consider something like GroupPrefix as its name. From a "Short" version, I expect something that substitutes a longer version of it, like in the context of command-line options, "-d" being a substitute for "--debug".
There was a problem hiding this comment.
Yes, in fact, it substitutes the longer version, i.e. ec2 for ec2.aws.crossplane.io. But I don't love ShortGroup either. Group only is misleading because the actual group is ec2.aws.crossplane.io.
The main motivation was that I didn't want to treat those two fields (GroupPrefix and GroupSuffix) as pure strings with those names because the concatenation we're doing now is not simply prepending anymore; we're opening a new group under the root group if short group is given. Open to suggestions for the name.
pkg/pipeline/run.go
Outdated
| for group, versions := range resourcesGroups { | ||
| for shortGroup, versions := range resourcesGroups { | ||
| group := pc.RootGroup | ||
| if shortGroup != "" { |
There was a problem hiding this comment.
We can consider trimming before comparison and concatenation.
There was a problem hiding this comment.
As synced, we can treat this as developer error if there is space there.
…g suffix anymore Signed-off-by: Muvaffak Onus <me@muvaf.com>
Description of your changes
Groupis actually the full API group likeec2.aws.crossplane.iobut it seems like the content we're expecting there is not of that form, so I changed its name. Also, I added a logic to allow""short group so that some resources can be in provider level group, likeResourceGroupin Azure where it should be intfazure.crossplane.io.I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
N/A