Skip to content

Create sdk package #221

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

Merged
merged 7 commits into from
Oct 21, 2021
Merged

Conversation

RedbackThomson
Copy link
Contributor

Description of changes:
Creates a new sdk package that strictly serves the purpose of loading the operations and shapes from the AWS SDK. This is useful to isolate creating the custom fields that need to be injected during this stage.

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

Copy link
Contributor

@vijtrip2 vijtrip2 left a comment

Choose a reason for hiding this comment

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

👍

pkg/sdk/op.go Outdated
@@ -11,7 +11,7 @@
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.

package model
package sdk
Copy link
Collaborator

Choose a reason for hiding this comment

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

this file should stay in pkg/model because it is part of ACK's model, not aws-sdk-go's model.

If you want to move line 42 to the pkg/sdk package, that's fine.

Copy link
Contributor Author

@RedbackThomson RedbackThomson Oct 13, 2021

Choose a reason for hiding this comment

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

I will probably need to move it into the util package, to resolve circular dependencies between pkg and model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it into a new operations package, instead. util feels more like helper methods than functional logic.

// Spec/Status subfield struct type name.
ConflictingNameSuffix = "_SDK"
)

// SDKHelper is a helper struct that helps work with the aws-sdk-go models and
// API model loader
type SDKHelper struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can call this just Helper now? And NewSDKHelper can be just NewHelper.

@RedbackThomson
Copy link
Contributor Author

/test dynamodb-controller-test

Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

looks good! Left one change request

koVarName string,
indentLevel int,
) string {
var op *awssdkmodel.Operation
switch opType {
case model.OpTypeGet:
case operations.OpTypeGet:
Copy link
Member

Choose a reason for hiding this comment

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

package operations should be renamed to operation and optionally we can rename OpTypeGet to TypeGet - https://rakyll.org/style-packages/

func NewSDKHelper(basePath string) *SDKHelper {
return &SDKHelper{
// NewHelper returns a new SDKHelper object
func NewHelper(basePath string) *Helper {
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Sorry @RedbackThomson I don't support the new operations package. That stuff belongs in the model package. Just move the definition of the OperationMap type to the new sdk package and make it unexported IMHO.

@RedbackThomson
Copy link
Contributor Author

As of this commit, I have merged SDKAPI back into the model package. I am using this type as an artifact produced by the sdk.Helper, which is now the only thing in the sdk package. Ideally the SDKAPI would be in there too, but it feels to tightly coupled to move at the moment.

@RedbackThomson
Copy link
Contributor Author

/test s3-controller-test
/test dynamodb-controller-test

Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Almost there :)

@@ -1314,7 +1314,7 @@ func varEmptyConstructorK8sType(
// setSDKForScalar returns the Go code that sets the value of a target variable
// or field to a scalar value. For target variables that are structs, we output
// the aws-sdk-go's common SetXXX() method. For everything else, we output
// normal assignment operations.
// normal assignment model.
Copy link
Collaborator

Choose a reason for hiding this comment

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

think this is a find/replace oopsie.

@@ -397,7 +397,7 @@ func setResourceReadMany(
// Find the element in the output shape that contains the list of
// resources. This heuristic is simplistic (just look for the field with a
// list type) but seems to be followed consistently by the aws-sdk-go for
// List operations.
// List model.
Copy link
Collaborator

Choose a reason for hiding this comment

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

think this is a find/replace oopsie.

@@ -307,7 +307,7 @@ func ListMemberNameInReadManyOutput(
// Find the element in the output shape that contains the list of
// resources. This heuristic is simplistic (just look for the field with a
// list type) but seems to be followed consistently by the aws-sdk-go for
// List operations.
// List model.
Copy link
Collaborator

Choose a reason for hiding this comment

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

think this is a find/replace oopsie.

Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

muy bueno, gracias @RedbackThomson!

@jaypipes
Copy link
Collaborator

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Oct 21, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaypipes, 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:
  • OWNERS [RedbackThomson,jaypipes,vijtrip2]

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 210b64d into aws-controllers-k8s:main Oct 21, 2021
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.

5 participants