-
Notifications
You must be signed in to change notification settings - Fork 251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Additional fields with defaults to generate.Config object #443
Conversation
99e2d13
to
a4cddc9
Compare
a4cddc9
to
c32a8d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @muvaf! :) Left a note inline about using the pkg/generate/config.Config
struct for this kind of information...
pkg/generate/generator.go
Outdated
@@ -38,7 +38,8 @@ type Generator struct { | |||
typeRenames map[string]string | |||
// Instructions to the code generator how to handle the API and its | |||
// resources | |||
cfg *ackgenconfig.Config | |||
cfg *ackgenconfig.Config | |||
stringStore map[ackmodel.StringIdentifier]string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this an attribute of pkg/generate/config.Config
instead of a member of the generator itself. This information (the starting prefixes for the Spec and Status fields and the nil find return type) is instructions to the generator on how to handle generated code, so it should go in the Config
.
You can also make it something a bit more structured than a map of strings. For example, you might use something like this:
type Config struct {
...
// Prefix stores any override string prefixes for fields in generated CRDs
Prefix *PrefixConfig `json:"prefix,omitempty"`
}
type PrefixConfig struct {
// SpecField stores the string prefix to use for fields in the Spec struct of
// a CRD. Defaults to `.Spec`
SpecField string `json:"spec_field"`
// StatusField stores the string prefix to use for fields in the Status struct of
// a CRD. Defaults to `.Status`
StatusField string `json:"status_field"`
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. As it currently stands, the list of strings is here https://github.com/aws/aws-controllers-k8s/pull/449/files#diff-bbfadd4328f26a3a50306ade20809a5d15a16a0803dc14d85463e0cce64cb4eaR27
I was kind of hesitant about whether it'd scale to have all custom strings strongly typed in the structs because the list seems to be growing and having them hard-coded works for both ack and crossplane. But making them part of Config
makes sense, too, in order to provide some flexibility to the users. I think we can have the current set of strings as hard-coded default for Config
object and overwrite what we read from generator.yaml
(if any) onto that Config
object. Let me know what you think after looking at the current list and I'll make the change.
pkg/model/crd.go
Outdated
@@ -732,14 +734,14 @@ func (r *CRD) GoCodeSetInput( | |||
sourceAdaptedVarName := sourceVarName | |||
crdField, found = r.SpecFields[renamedName] | |||
if found { | |||
sourceAdaptedVarName += ".Spec" | |||
sourceAdaptedVarName += r.StringStore[SpecFieldPath] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the map[string]string
here, if you went with the PrefixConfig
struct I suggest above, you could have little helper methods on CRD
like so:
func (crd *CRD) specFieldPrefix() string {
if crd.cfg == nil || crd.cfg.Prefix == nil {
return ".Spec"
}
return c.Prefix.SpecField
}
func (crd *CRD) statusFieldPrefix() string {
if crd.cfg == nil || crd.cfg.Prefix == nil {
return ".Status"
}
return c.Prefix.StatusField
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that for both ack and crossplane, we'd have a hard-coded default Config
object and use the generator.yaml
as an overlay on top of that Config
so we'd make sure there is always a value in those fields.
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
c32a8d5
to
ad1fd3a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaypipes Now they are part of the config object except for the APIGroupSuffix
because that one is processed in SDKAPI
object. I plan to change its value in cmd/crossplane.go
after it's initialized. For ACK, nothing really changes.
@@ -1855,16 +1858,6 @@ func (r *CRD) goCodeSetOutputReadMany( | |||
// operation by checking for matching values in these fields. | |||
matchFieldNames := r.listOpMatchFieldNames() | |||
|
|||
// if len(resp.CacheClusters) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If len(resp.CacheClusters) == 0
, the for
loop won't run at all, meaning found
will be false
and there is a return statement after the for
loop that returns the same answer if found == false
. So, behavior shouldn't change when we remove this if block.
@@ -315,6 +315,9 @@ func (r *CRD) UnpackAttributes() { | |||
// IsPrimaryARNField returns true if the supplied field name is likely the resource's | |||
// ARN identifier field. | |||
func (r *CRD) IsPrimaryARNField(fieldName string) bool { | |||
if r.genCfg != nil && !r.genCfg.IncludeACKMetadata { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaypipes since we know that it will always have some value, what would you think about making r.genCfg
a value variable instead of pointer? We wouldn't need to make if r.genCfg != nil
check everywhere and run into risk of panicing in places we don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesomeness. You rock @muvaf :)
Description of changes: As part of the ongoing effort about generating Crossplane AWS Provider, there are some small changes where ACK and Crossplane patterns diverge. In CP, we have all the provider-related fields under
spec.forProvider
so that it's separate from CP machinery fields. Same with status, we havestatus.atProvider
to differentiate them from other fields. For details, you can see Managed Resources API Patterns Design Doc.I tried to make the change as least intrusive as possible to the existing mechanisms. Please let me know if you think it could be done in a better way.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.