Update Runtime to v0.13.0 #28

mbaijal · 2021-08-25T06:31:30Z

Update Runtime to v0.13.0 and add the primary identifier field for both ScalableTarget and ScalingPolicy as required.

Testing

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

RedbackThomson

Thank you for doing this in a separate PR. Saves 700 lines of confusion for me in the next PR.

/lgtm

surajkota · 2021-08-25T19:24:48Z

/hold

surajkota

few questions and comments

One more thing to note is there are some bug fixes in aws-controllers-k8s/runtime#48 and aws-controllers-k8s/code-generator#168 which might affect these resources as well. So we should proceed accordingly

go.sum

go.mod

surajkota · 2021-08-25T18:59:01Z

pkg/resource/scalable_target/resource.go

+		r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{}
 	}
-	r.ko.Spec.ResourceID = identifier.NameOrID
+	r.ko.Status.ACKResourceMetadata.ARN = identifier.ARN


its using ARN instead of the primary_identified field from generator.yaml, a possible bug?

Also coming from here

so this is a bug then?
the identifier for this resource is resourceID right? in fact this resource does not have an ARN

The SetResourceIdentifiers previously could not detect the primary identifier field for ReadMany describe operations (in this case DescribeScalableTargets). I have since rewritten it (pending aws-controllers-k8s/code-generator#170) to do so. Once that PR is merge, the override in the generator config will not be necessary.

For this particular problem, I would recommend trying ResourceIds as the primary_identifier_field_name, since this is the field in the describe call that we hope to reference.

@RedbackThomson resourceIds is already set in primary_identifier_field_name right, any idea why code still pointing to ARN?

we can have this discussion on a separate PR to unblock you - #29 (comment)

I can approve, let me know.

ResourceID is the value of primary_identifier_field_name - I am recommending ResourceIds. The field needs to match the member of the describe input shape.

@mbaijal can you make this change so we can merge this PR?

pkg/resource/scalable_target/sdk.go

pkg/resource/scaling_policy/sdk.go

helm/values.yaml

helm/templates/_helpers.tpl

helm/values.yaml

mbaijal · 2021-08-30T21:28:14Z

/test applicationautoscaling-kind-e2e

This reverts commit 73b537a.

surajkota · 2021-08-31T17:06:21Z

/approve

#28 (comment)
can continue on a separate PR

a-hilaly

LGTM 👍
Left few comments

pkg/testutil/test_suite_runner.go

a-hilaly · 2021-09-01T13:07:57Z

pkg/resource/scalable_target/manager_test_suite_test.go

+	"github.com/aws-controllers-k8s/applicationautoscaling-controller/pkg/testutil"
 	mocksvcsdkapi "github.com/aws-controllers-k8s/applicationautoscaling-controller/test/mocks/aws-sdk-go/applicationautoscaling"
 	ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1"
-	"github.com/ghodss/yaml"
-	"github.com/aws-controllers-k8s/applicationautoscaling-controller/pkg/testutil"
+	ackmetrics "github.com/aws-controllers-k8s/runtime/pkg/metrics"
 	acktypes "github.com/aws-controllers-k8s/runtime/pkg/types"
 	svcsdk "github.com/aws/aws-sdk-go/service/applicationautoscaling"
+	"github.com/ghodss/yaml"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"go.uber.org/zap/zapcore"
 	"path/filepath"
-	"testing"
 	ctrlrtzap "sigs.k8s.io/controller-runtime/pkg/log/zap"
-	ackmetrics "github.com/aws-controllers-k8s/runtime/pkg/metrics"
-	"go.uber.org/zap/zapcore"
+	"testing"


Needs goimports with probably some manual reorganization

Amine, we will address this in a separate PR.

a-hilaly · 2021-09-01T13:08:13Z

pkg/resource/scaling_policy/manager_test_suite_test.go

+	"github.com/aws-controllers-k8s/applicationautoscaling-controller/pkg/testutil"
 	mocksvcsdkapi "github.com/aws-controllers-k8s/applicationautoscaling-controller/test/mocks/aws-sdk-go/applicationautoscaling"
 	ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1"
-	"github.com/ghodss/yaml"
-	"github.com/aws-controllers-k8s/applicationautoscaling-controller/pkg/testutil"
+	ackmetrics "github.com/aws-controllers-k8s/runtime/pkg/metrics"
 	acktypes "github.com/aws-controllers-k8s/runtime/pkg/types"
 	svcsdk "github.com/aws/aws-sdk-go/service/applicationautoscaling"
+	"github.com/ghodss/yaml"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"go.uber.org/zap/zapcore"
 	"path/filepath"
-	"testing"
 	ctrlrtzap "sigs.k8s.io/controller-runtime/pkg/log/zap"
-	ackmetrics "github.com/aws-controllers-k8s/runtime/pkg/metrics"
-	"go.uber.org/zap/zapcore"
+	"testing"
 )


surajkota

/approve

surajkota · 2021-09-01T18:21:48Z

/lgtm

ack-bot · 2021-09-01T18:21:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akartsky, mbaijal, RedbackThomson, surajkota

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,akartsky,mbaijal,surajkota]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

surajkota · 2021-09-01T18:23:57Z

/hold cancel

ack-bot requested review from a-hilaly and vijtrip2 August 25, 2021 06:31

ack-bot added the approved label Aug 25, 2021

mbaijal changed the title ~~Update Runtime to v0.12.0~~ [wip] Update Runtime to v0.12.0 Aug 25, 2021

ack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2021

Update Runtime to v0.12.0

be14390

mbaijal force-pushed the update-runtime-0.12 branch from 0ffabf4 to be14390 Compare August 25, 2021 09:52

mbaijal changed the title ~~[wip] Update Runtime to v0.12.0~~ Update Runtime to v0.12.0 Aug 25, 2021

ack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2021

akartsky approved these changes Aug 25, 2021

View reviewed changes

RedbackThomson approved these changes Aug 25, 2021

View reviewed changes

ack-bot assigned RedbackThomson Aug 25, 2021

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2021

ack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 25, 2021

surajkota requested changes Aug 25, 2021

View reviewed changes

ack-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2021

a-hilaly reviewed Aug 25, 2021

View reviewed changes

helm/values.yaml Outdated Show resolved Hide resolved

jaypipes suggested changes Aug 26, 2021

View reviewed changes

helm/values.yaml Outdated Show resolved Hide resolved

helm/values.yaml Show resolved Hide resolved

Review Comments and unit test failure

4d03f0e

mbaijal force-pushed the update-runtime-0.12 branch from e21169d to 4d03f0e Compare August 27, 2021 08:34

Update Runtime to v0.13.0

73b537a

mbaijal changed the title ~~Update Runtime to v0.12.0~~ Update Runtime to v0.13.0 Aug 30, 2021

Revert "Update Runtime to v0.13.0"

6fb2fff

This reverts commit 73b537a.

mbaijal changed the title ~~Update Runtime to v0.13.0~~ Update Runtime to v0.12.0 Aug 30, 2021

ryansteakley mentioned this pull request Sep 1, 2021

Enable updates for ScalableTarget and ScalingPolicy [on branch update-runtime-0.12] #30

Closed

a-hilaly reviewed Sep 1, 2021

View reviewed changes

Update runtime to v0.13.0 again

50a10e0

surajkota changed the title ~~Update Runtime to v0.12.0~~ Update Runtime to v0.13.0 Sep 1, 2021

surajkota approved these changes Sep 1, 2021

View reviewed changes

ack-bot assigned surajkota Sep 1, 2021

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2021

ack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2021

ack-bot merged commit ecf179d into aws-controllers-k8s:main Sep 1, 2021

Update Runtime to v0.13.0 #28

Update Runtime to v0.13.0 #28

Uh oh!

Conversation

mbaijal commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

RedbackThomson left a comment

Choose a reason for hiding this comment

Uh oh!

surajkota commented Aug 25, 2021

Uh oh!

surajkota left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mbaijal commented Aug 30, 2021

Uh oh!

surajkota commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surajkota left a comment

Choose a reason for hiding this comment

Uh oh!

surajkota commented Sep 1, 2021

Uh oh!

ack-bot commented Sep 1, 2021

Uh oh!

surajkota commented Sep 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mbaijal commented Aug 25, 2021 •

edited

Loading

surajkota commented Aug 31, 2021 •

edited

Loading