Skip to content
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

Fix panic in code generator when generating lambda controller #384

Merged

Conversation

a-hilaly
Copy link
Member

@a-hilaly a-hilaly commented Dec 18, 2022

The recent patch (PR 383) introduced a regression that caused the
code generator to panic when generating the lambda controller.
This commit addresses the issue by adding a check to ensure that
the shapeRef reference is not nil before accessing the
Documentation field. This prevents the panic from occurring and
allows the code generator to function correctly.

By submitting this pull request, I confirm that my contribution is
made under the terms of the Apache 2.0 license.

The recent patch (PR 383) introduced a regression that caused the
code generator to panic when generating the lambda controller.
This commit addresses the issue by adding a check to ensure that
the shapeRef reference is not nil before accessing the
Documentation field. This prevents the panic from occurring and
allows the code generator to function correctly.
@@ -16,7 +16,7 @@ import (
{{ .CRD.Documentation }}
type {{ .CRD.Kind }}Spec struct {
{{ range $fieldName, $field := .CRD.SpecFields }}
{{ if $field.ShapeRef.Documentation -}}
{{ if $field.ShapeRef -}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you nil-check both ShapeRef and ShapeRef.Documentation here?

Copy link
Member Author

@a-hilaly a-hilaly Dec 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ShapeRef.Documentation is of type string (not *string), hence it will never cause nil pointer panics. Also this was the original code in https://github.com/aws-controllers-k8s/code-generator/pull/383/files

Copy link
Collaborator

@jaypipes jaypipes Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR broke the SNS and SQS controller's API generation.

After checking out latest code-generator and re-running make build-controller SERVICE=sns, I get this git diff.

[jaypipes@thelio sns-controller]$ git diff
diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml
index 81fbab0..a894215 100755
--- a/apis/v1alpha1/ack-generate-metadata.yaml
+++ b/apis/v1alpha1/ack-generate-metadata.yaml
@@ -1,9 +1,9 @@
 ack_generate_info:
-  build_date: "2022-12-16T20:24:15Z"
-  build_hash: e661ce95afc39b380653ca655503daebf1e1831b
+  build_date: "2022-12-19T18:13:35Z"
+  build_hash: 8d843f48df4504e3d19712c0e27dbb388ff40cc0
   go_version: go1.18.1
-  version: v0.21.0-5-ge661ce9
-api_directory_checksum: a0c05230761bda067a0f03b794041014296b8bcb
+  version: v0.21.0-6-g8d843f4
+api_directory_checksum: b10135ae96fc0459ddaa92c9ea9429424b052424
 api_version: v1alpha1
 aws_sdk_go_version: v1.44.93
 generator_config_info:
diff --git a/apis/v1alpha1/platform_application.go b/apis/v1alpha1/platform_application.go
index e0402de..9e757fe 100644
--- a/apis/v1alpha1/platform_application.go
+++ b/apis/v1alpha1/platform_application.go
@@ -24,10 +24,14 @@ import (
 //
 // Platform application object.
 type PlatformApplicationSpec struct {
-       EventDeliveryFailure   *string `json:"eventDeliveryFailure,omitempty"`
-       EventEndpointCreated   *string `json:"eventEndpointCreated,omitempty"`
-       EventEndpointDeleted   *string `json:"eventEndpointDeleted,omitempty"`
-       EventEndpointUpdated   *string `json:"eventEndpointUpdated,omitempty"`
+       EventDeliveryFailure *string `json:"eventDeliveryFailure,omitempty"`
+
+       EventEndpointCreated *string `json:"eventEndpointCreated,omitempty"`
+
+       EventEndpointDeleted *string `json:"eventEndpointDeleted,omitempty"`
+
+       EventEndpointUpdated *string `json:"eventEndpointUpdated,omitempty"`
+
        FailureFeedbackRoleARN *string `json:"failureFeedbackRoleARN,omitempty"`
        // Application names must be made up of only uppercase and lowercase ASCII letters,
        // numbers, underscores, hyphens, and periods, and must be between 1 and 256
@@ -38,10 +42,14 @@ type PlatformApplicationSpec struct {
        // (Apple Push Notification Service), APNS_SANDBOX, and GCM (Firebase Cloud
        // Messaging).
        // +kubebuilder:validation:Required
-       Platform                  *string `json:"platform"`
-       PlatformCredential        *string `json:"platformCredential,omitempty"`
-       PlatformPrincipal         *string `json:"platformPrincipal,omitempty"`
-       SuccessFeedbackRoleARN    *string `json:"successFeedbackRoleARN,omitempty"`
+       Platform *string `json:"platform"`
+
+       PlatformCredential *string `json:"platformCredential,omitempty"`
+
+       PlatformPrincipal *string `json:"platformPrincipal,omitempty"`
+
+       SuccessFeedbackRoleARN *string `json:"successFeedbackRoleARN,omitempty"`
+
        SuccessFeedbackSampleRate *string `json:"successFeedbackSampleRate,omitempty"`
 }
 
diff --git a/apis/v1alpha1/platform_endpoint.go b/apis/v1alpha1/platform_endpoint.go
index e24fd42..b809a73 100644
--- a/apis/v1alpha1/platform_endpoint.go
+++ b/apis/v1alpha1/platform_endpoint.go
@@ -23,12 +23,14 @@ import (
 // PlatformEndpointSpec defines the desired state of PlatformEndpoint.
 type PlatformEndpointSpec struct {
        CustomUserData *string `json:"customUserData,omitempty"`
-       Enabled        *string `json:"enabled,omitempty"`
+
+       Enabled *string `json:"enabled,omitempty"`
        // PlatformApplicationArn returned from CreatePlatformApplication is used to
        // create a an endpoint.
        // +kubebuilder:validation:Required
        PlatformApplicationARN *string `json:"platformApplicationARN"`
-       Token                  *string `json:"token,omitempty"`
+
+       Token *string `json:"token,omitempty"`
 }
 
 // PlatformEndpointStatus defines the observed state of PlatformEndpoint
diff --git a/apis/v1alpha1/topic.go b/apis/v1alpha1/topic.go
index 31d1506..87b35f7 100644
--- a/apis/v1alpha1/topic.go
+++ b/apis/v1alpha1/topic.go
@@ -34,9 +34,12 @@ type TopicSpec struct {
        //
        // Length Constraints: Maximum length of 30,720.
        DataProtectionPolicy *string `json:"dataProtectionPolicy,omitempty"`
-       DeliveryPolicy       *string `json:"deliveryPolicy,omitempty"`
-       DisplayName          *string `json:"displayName,omitempty"`
-       KMSMasterKeyID       *string `json:"kmsMasterKeyID,omitempty"`
+
+       DeliveryPolicy *string `json:"deliveryPolicy,omitempty"`
+
+       DisplayName *string `json:"displayName,omitempty"`
+
+       KMSMasterKeyID *string `json:"kmsMasterKeyID,omitempty"`
        // The name of the topic you want to create.
        //
        // Constraints: Topic names must be made up of only uppercase and lowercase
@@ -45,7 +48,8 @@ type TopicSpec struct {
        //
        // For a FIFO (first-in-first-out) topic, the name must end with the .fifo suffix.
        // +kubebuilder:validation:Required
-       Name   *string `json:"name"`
+       Name *string `json:"name"`
+
        Policy *string `json:"policy,omitempty"`

@jljaco
Copy link
Contributor

jljaco commented Dec 19, 2022

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2022
@ack-bot
Copy link
Collaborator

ack-bot commented Dec 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jljaco

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-bot ack-bot merged commit 8d843f4 into aws-controllers-k8s:main Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants