Capacity reservations feature implementation #235
Conversation
…D operations for Tags
|
Hi @fizashaikh. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
ack-prow
bot
added
the
needs-ok-to-test
fizashaikh
changed the title
|
/ok-to-test |
ack-prow
bot
added
ok-to-test
|
at first glance, this LGTM, @michaelhtm can you PTAL |
|
/test ec2-verify-attribution |
There was a problem hiding this comment.
Hey @fizashaikh, thanks for contributing!
Everything lgtm, can you please resolve the conflict?
Thanks
Thanks for reviewing @michaelhtm ! All conflicts are resolved now 👍 |
There was a problem hiding this comment.
Hi @fizashaikh. apologies for the back and forth. I have a couple more comments about this PR. Thanks!
| EndDate *metav1.Time `json:"endDate,omitempty"` | ||
| // Indicates the way in which the Capacity Reservation ends. A Capacity Reservation | ||
| // can have one of the following end types: | ||
| // | ||
| // - unlimited - The Capacity Reservation remains active until you explicitly | ||
| // cancel it. Do not provide an EndDate if the EndDateType is unlimited. | ||
| // | ||
| // - limited - The Capacity Reservation expires automatically at a specified | ||
| // date and time. You must provide an EndDate value if the EndDateType value | ||
| // is limited. | ||
| EndDateType *string `json:"endDateType,omitempty"` |
There was a problem hiding this comment.
nit: not sure if this should be required, but can we have custom code validating the assignment of these fields?
| // | ||
| // You can request a future-dated Capacity Reservation between 5 and 120 days | ||
| // in advance. | ||
| StartDate *metav1.Time `json:"startDate,omitempty"` |
There was a problem hiding this comment.
î same for this one? with end-date
|
Nice 🚀 🚀 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fizashaikh, michaelhtm 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 |
|
/hold |
ack-prow
bot
added
the
do-not-merge/hold
| latestCopy, err := rm.sdkFind(ctx, desired) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| ko.Status.AvailableInstanceCount = latestCopy.ko.Status.AvailableInstanceCount | ||
| ko.Status.TotalInstanceCount = latestCopy.ko.Status.TotalInstanceCount | ||
| ko.Status.State = latestCopy.ko.Status.State |
There was a problem hiding this comment.
late initialize https://aws-controllers-k8s.github.io/community/docs/contributor-docs/code-generator-config/#late_initialize-late-initialization-of-a-field solves this without custom code..
There was a problem hiding this comment.
Thanks for reviewing the PR @michaelhtm !
Looks like late initialization code generator looks for these fields in Spec, however I am trying to update the fields within Status to reflect the correct values of these fields after a capacity reservation is modified/updated.
Therefore, I see errors when I remove the custom hook and update generator.yaml to include these fields with late_initialization,
CapacityReservation:
fields:
State:
print:
name: STATE
late_initialize: {}
AvailableInstanceCount:
late_initialize: {}
TotalInstanceCount:
late_initialize: {}
The code generated with above change looks like this,
// incompleteLateInitialization return true if there are fields which were supposed to be
// late initialized but are not. If all the fields are late initialized, false is returned
func (rm *resourceManager) incompleteLateInitialization(
res acktypes.AWSResource,
) bool {
ko := rm.concreteResource(res).ko.DeepCopy()
if ko.Spec.AvailableInstanceCount == nil {
return true
}
if ko.Spec.State == nil {
return true
}
if ko.Spec.TotalInstanceCount == nil {
return true
}
return false
}
Error,
ko.Spec.AvailableInstanceCount undefined
(type "github.com/aws-controllers-k8s/ec2-controller/apis/v1alpha1".CapacityReservationSpec
has no field or method AvailableInstanceCount)
Am I missing something? Is there a way to tell code generator to look for these fields in Status?
Thanks!
There was a problem hiding this comment.
You are right..just looked into the implementation of late_initialize and it looks like it only supports Spec fields..it should be an easy fix if you want to tackle that as well, maybe even by allowing user to specify whether late_initialize is in spec or status...
https://github.com/aws-controllers-k8s/code-generator/blob/8762917215d9902b2011a2b0b1b0c776855a683e/pkg/generate/code/late_initialize.go#L302-L306
ack-prow
bot
removed
the
do-not-merge/hold
Issue: aws-controllers-k8s/community#2234
Description of changes:
This change implements capacity reservations feature in ec2 operator.
It adds e2e tests for the capacity reservations CRUD functionality.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.