Skip to content

Conversation

mbaijal
Copy link
Contributor

@mbaijal mbaijal commented Sep 7, 2021

Issue #, if available:
aws-controllers-k8s/community#932

Description of changes:
Add creationTime to Status for ScalableTarget and ScalingPolicy resources based off of code gen changes in this PR.

TODO: Add required check to the existing integration test.

Testing
Manually tested that the field is present in the Status of the kubectl get response.
Kind tests pass locally

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

Copy link
Contributor

@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.

Nice addition, @mbaijal, thanks!

Copy link
Member

@surajkota surajkota left a comment

Choose a reason for hiding this comment

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

+1 need to make sure creation and last modified time are reported in the same time zone

if they are not, we can overwrite the creation time in custom code similar to modified time

@mbaijal mbaijal changed the title [testing-in-progress] Add creationTime to Status for ScalableTarget and ScalingPolicy resources Add creationTime to Status for ScalableTarget and ScalingPolicy resources Sep 7, 2021
ack-bot pushed a commit to aws-controllers-k8s/code-generator that referenced this pull request Sep 8, 2021
…183)

Issue #, if available:
aws-controllers-k8s/community#932

Description of changes:
`getMemberByPath` checks the shape type as it loops through members to ensure lists are handled correctly. Currently I have simply ignored the empty element created by the splitting of the string on the single dot instead of handling the double dot notation explicitly. Open to comments. 
Also added a Unit Test for the same. 

Testing
1. Generated code for ApplicationAutoscaling and ensured CreationTime field now works as expected. Linked changes [in this PR](aws-controllers-k8s/applicationautoscaling-controller#39).
2. Ensured that the SageMaker Controller can be generated using new changes with no diff as expected. 

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@mbaijal
Copy link
Contributor Author

mbaijal commented Sep 8, 2021

+1 need to make sure creation and last modified time are reported in the same time zone

if they are not, we can overwrite the creation time in custom code similar to modified time

Sure, but I am creating a separate PR for that since this is also a demo of the associated Code-gen change.

Copy link
Contributor

@RedbackThomson RedbackThomson left a comment

Choose a reason for hiding this comment

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

Great!
/lgtm

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

ack-bot commented Sep 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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 175fa03 into aws-controllers-k8s:main Sep 8, 2021
ack-bot pushed a commit that referenced this pull request Sep 13, 2021
Description of changes:
release artifacts for release v0.0.3

Release Notes draft:
This release includes the following resources and updates:
 - Update ACK Runtime from 0.13.0 to 0.13.2 ( #41 )
    - Please refer to https://github.com/aws-controllers-k8s/runtime/releases for a detailed list of changes
 - The Status field now includes the CreationTime as recorded by the server ( #39 ).
 - The Status field also includes the `LastModifiedTime` which records the time the resource was last updated ( #40 ).
 - Includes updates to the README file ( #37 )


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
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