Skip to content

Conversation

ryansteakley
Copy link
Member

@ryansteakley ryansteakley commented Oct 8, 2021

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

Description of changes:
Update to codegen v0.15.1 and use runtime v0.15.1 in the runtime

  • Uses is_primary_arn instead of primary_identifier_field
  • ResourceGK was renamed to GroupKind so changed this field wherever we used it.

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

@ack-bot ack-bot requested a review from RedbackThomson October 8, 2021 20:26
@ryansteakley ryansteakley force-pushed the update-v0.15.1 branch 2 times, most recently from 1c53805 to 420a8d5 Compare October 8, 2021 20:46
RetainAllVariantProperties: true
DescribeModelPackage:
custom_check_required_fields_missing_method: customCheckRequiredFieldsMissingMethod
primary_identifier_field_name: ARN
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this override in the new format. can you elaborate on how it works? because I see that model_package is_identifier has not changed

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey Suraj, here it is for your reference aws-controllers-k8s/code-generator#190
The code it generates is the same but the way to enable it has changed. From setting primary_identifier_field_name: ARN to setting it as is_arn_primary_key: true

Copy link
Member

Choose a reason for hiding this comment

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

Sg, I missed the diff b/w is_primary_key: true to is_arn_primary_key: true

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.

/approve
/lgtm

RetainAllVariantProperties: true
DescribeModelPackage:
custom_check_required_fields_missing_method: customCheckRequiredFieldsMissingMethod
primary_identifier_field_name: ARN
Copy link
Member

Choose a reason for hiding this comment

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

Sg, I missed the diff b/w is_primary_key: true to is_arn_primary_key: true

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

ack-bot commented Oct 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akartsky, mbaijal, ryansteakley, 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 [akartsky,mbaijal,ryansteakley,surajkota]

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 780280d into aws-controllers-k8s:main Oct 18, 2021
ack-bot pushed a commit that referenced this pull request Oct 22, 2021
Release Notes Draft:
- Unit tests for all resources currently included in [pkg/resource](https://github.com/aws-controllers-k8s/sagemaker-controller/tree/main/pkg/resource)
- Helm image is updated to be Helm 3.7 compatible #119
  -  ⚠️ **Breaking Change** Helm versions < `3.7` are no longer compatible
- Update ACK Runtime from `0.14.0` to `0.15.1` #120
  - ⚠️ **Breaking Change** `aws-account-id` can no longer be set and has been removed from the charts.
  - Please refer to https://github.com/aws-controllers-k8s/runtime/releases for a detailed list of changes 

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.

6 participants