Skip to content
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

Proposal for handling late-initialization in ACK. #849

Merged
merged 3 commits into from
Jul 28, 2021

Conversation

vijtrip2
Copy link
Contributor

Issue #812 , if available:

Description of changes:

  • Adding the proposal on how to handle server-side defaults/late-initialization in ACK.
  • You can find the rendered proposal document here

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 review from jaypipes and mhausenblas June 28, 2021 17:10
@vijtrip2 vijtrip2 self-assigned this Jun 28, 2021
@vijtrip2
Copy link
Contributor Author

/hold
/do-not-merge

@ack-bot ack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2021
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.

Wow, really good stuff, here. Your solution is actually a lot more elegant than I had initially imagined. Great detective work with flux. Left some comments nitpicking specifics

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.

@vijtrip2 I would like you to break this PR into two separate proposals, one that specifically addresses the late initialization of fields that the user does not specify and the server sets to a default value and another proposal for handling fields where the k8s user does specific a value for the field, but the backend service overrides that specified value.

The two use cases are substantially different and I'd like to tackle the former use case (defaulting field values that the k8s user did not specify) ASAP before moving on to tacklling the server-override field use case.

@vijtrip2
Copy link
Contributor Author

vijtrip2 commented Jul 1, 2021

@vijtrip2 I would like you to break this PR into two separate proposals, one that specifically addresses the late initialization of fields that the user does not specify and the server sets to a default value and another proposal for handling fields where the k8s user does specific a value for the field, but the backend service overrides that specified value.

The two use cases are substantially different and I'd like to tackle the former use case (defaulting field values that the k8s user did not specify) ASAP before moving on to tackling the server-override field use case.

Sounds good. I actually started implementing server-override, that's why this PR went stale.
I will prioritize work on server-side defaults instead.

@echen-98
Copy link
Contributor

echen-98 commented Jul 2, 2021

@vijtrip2 I would like you to break this PR into two separate proposals, one that specifically addresses the late initialization of fields that the user does not specify and the server sets to a default value and another proposal for handling fields where the k8s user does specific a value for the field, but the backend service overrides that specified value.

The two use cases are substantially different and I'd like to tackle the former use case (defaulting field values that the k8s user did not specify) ASAP before moving on to tacklling the server-override field use case.

+1, I think "default fields" and "override fields" need different solutions.

I would request that this comment be considered when coming up with solutions for default fields.

@vijtrip2
Copy link
Contributor Author

vijtrip2 commented Jul 2, 2021

@vijtrip2 I would like you to break this PR into two separate proposals, one that specifically addresses the late initialization of fields that the user does not specify and the server sets to a default value and another proposal for handling fields where the k8s user does specific a value for the field, but the backend service overrides that specified value.

The two use cases are substantially different and I'd like to tackle the former use case (defaulting field values that the k8s user did not specify) ASAP before moving on to tacklling the server-override field use case.

+1, I think "default fields" and "override fields" need different solutions.

I would request that this comment be considered when coming up with solutions for default fields.

@echen-98 We decided in a previous meeting that late initialized fields will be populated back in resource spec. And the new solution will be based on that for late initialized(i.e.default) fields. More details will be there in future PR.

Going that path, "the comment" regarding nil difference will not be required for handling unnecessary updates.

@vijtrip2 vijtrip2 force-pushed the late-initializers branch from 892971e to 135eaa7 Compare July 6, 2021 22:45
@vijtrip2
Copy link
Contributor Author

vijtrip2 commented Jul 6, 2021

  • I have updated this PR to only deal with late initialized fields
  • I have created a separate PR #856 to propose a solution for server-side-overrides.

@vijtrip2 vijtrip2 force-pushed the late-initializers branch from 135eaa7 to fb03992 Compare July 7, 2021 17:43
@vijtrip2 vijtrip2 force-pushed the late-initializers branch from fb03992 to efe9635 Compare July 8, 2021 20:53
@vijtrip2 vijtrip2 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2021
@vijtrip2 vijtrip2 force-pushed the late-initializers branch from efe9635 to c7fcc69 Compare July 8, 2021 20:57
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.

@vijtrip2 I'd like to see a fairly substantial reworking of this proposal document. See inline for details, but overall, I'd like to see the following layout used:

# Problem overview
# Solution requirements
# Proposed solution
## ACK code-generator
## ACK runtime
# Alternate solutions considered

@vijtrip2 vijtrip2 force-pushed the late-initializers branch from c7fcc69 to cbeb99a Compare July 14, 2021 20:38
@vijtrip2 vijtrip2 requested a review from jaypipes July 15, 2021 16:32
@vijtrip2 vijtrip2 force-pushed the late-initializers branch from cbeb99a to 849b5cd Compare July 16, 2021 17:22
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.

@vijtrip2 I think you may have misunderstood previous comments I made. I've tried to highlight in the inline comments how I think the changes to the reconciler should be done in order to minimize changes needed to the AWSResourceManager interface...

@vijtrip2 vijtrip2 force-pushed the late-initializers branch from 849b5cd to e716995 Compare July 20, 2021 20:53
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.

This is much better. Getting close. A few suggested renames inline...

@vijtrip2 vijtrip2 force-pushed the late-initializers branch from e716995 to 29e5afc Compare July 20, 2021 23:32
@vijtrip2 vijtrip2 requested a review from jaypipes July 20, 2021 23:34
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.

I'm good with this. If @RedbackThomson and @a-hilaly are also good with it, let's merge away.

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 solid to me! I'm good with merging this PR once @RedbackThomson comments are addressed

@vijtrip2 vijtrip2 force-pushed the late-initializers branch from f12d486 to ddee615 Compare July 28, 2021 15:36
@RedbackThomson
Copy link
Contributor

/lgtm

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

ack-bot commented Jul 28, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, 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 [A-Hilaly,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 b784d04 into aws-controllers-k8s:main Jul 28, 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.

7 participants