Skip to content

Proposal for handling server-side-overrides in ACK.#856

Closed
vijtrip2 wants to merge 1 commit intoaws-controllers-k8s:mainfrom
vijtrip2:server-side-override
Closed

Proposal for handling server-side-overrides in ACK.#856
vijtrip2 wants to merge 1 commit intoaws-controllers-k8s:mainfrom
vijtrip2:server-side-override

Conversation

@vijtrip2
Copy link
Contributor

@vijtrip2 vijtrip2 commented Jul 6, 2021

Issue #855

Description of changes:

  • Proposal for handling server-side overrides in ACK controllers.
  • The solution will help with handling the differences between desired and latest state of custom resource due to server side overrides during reconciliation loop.
  • The solution will also provide a way to communicate to k8s user that server-side overrides have taken place for a custom resource.
  • The solution will help AWS service teams simplify the custom delta logic, which they currently use to handle server-side overrides.

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

@vijtrip2 vijtrip2 self-assigned this Jul 6, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Jul 6, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vijtrip2

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@vijtrip2 vijtrip2 mentioned this pull request Jul 6, 2021
Merged
@vijtrip2 vijtrip2 force-pushed the server-side-override branch from 678ec3c to 3ff15a3 Compare August 3, 2021 22:43
@vijtrip2 vijtrip2 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 4, 2021
@vijtrip2 vijtrip2 force-pushed the server-side-override branch from 3ff15a3 to 41985da Compare August 4, 2021 01:54
RedbackThomson
RedbackThomson reviewed Aug 6, 2021
View reviewed changes
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.

Seems solid. Added some questions inline

docs/design/proposals/server-side-override/server-side-override.md
conditions:
- type: ACK.ServerSideOverride
status: True
reason: ServerSideSpecOverride
Copy link
Contributor

@RedbackThomson RedbackThomson Aug 6, 2021

Choose a reason for hiding this comment

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

Could probably just be shortened to SpecOverride - ServerSide is implied

docs/design/proposals/server-side-override/server-side-override.md
- type: ACK.ServerSideOverride
status: True
reason: ServerSideSpecOverride
message: Spec override values are present in services.aws.k8s/spec-overrides annotation
Copy link
Contributor

@RedbackThomson RedbackThomson Aug 6, 2021

Choose a reason for hiding this comment

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

Could this message include a list of overridden keys, saving users the hassle of looking up the annotations.

docs/design/proposals/server-side-override/server-side-override.md
Comment on lines +91 to +93
services.k8s.aws/spec-overrides:
“{
\“overrideFieldB\”:
Copy link
Contributor

@RedbackThomson RedbackThomson Aug 6, 2021

Choose a reason for hiding this comment

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

nit: formatting for this document - maybe add Annotations: like you have for Spec: and Status:

docs/design/proposals/server-side-override/server-side-override.md
}”
Spec:
fieldA: valueA
overrideFieldB: updatedValueB-xyz
Copy link
Contributor

@RedbackThomson RedbackThomson Aug 6, 2021

Choose a reason for hiding this comment

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

This should be updatedValueB, right? We don't patch over the user's Spec input?

echen-98
echen-98 reviewed Aug 9, 2021
View reviewed changes
docs/design/proposals/server-side-override/server-side-override.md

## Solution

In this solution, If an existing value is updated as part of Create/Update API output, ACK service controller will persist
Copy link
Contributor

@echen-98 echen-98 Aug 9, 2021

Choose a reason for hiding this comment

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

I know this may be obvious to some, but it may be useful to make explicit why the annotation/mapping is only written during Creates/Updates, and not during Reads

@vijtrip2 vijtrip2 mentioned this pull request Nov 8, 2021
Merged
@ack-bot
Copy link
Collaborator

ack-bot commented Nov 17, 2021

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

@ack-bot ack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 17, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Dec 17, 2021

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle rotten

@ack-bot ack-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 17, 2021
@a-hilaly
Copy link
Member

a-hilaly commented Dec 20, 2021

/remove-lifecycle rotten

@ack-bot ack-bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Dec 20, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Mar 20, 2022

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

@ack-bot ack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 20, 2022
@vijtrip2 vijtrip2 removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 21, 2022
@ack-bot
Copy link
Collaborator

ack-bot commented Jun 19, 2022

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

@ack-bot ack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 19, 2022
@vijtrip2 vijtrip2 closed this Jun 20, 2022
@jlbutler jlbutler mentioned this pull request Jun 5, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaypipes jaypipes Awaiting requested review from jaypipes

@a-hilaly a-hilaly Awaiting requested review from a-hilaly

2 more reviewers

@RedbackThomson RedbackThomson RedbackThomson left review comments

@echen-98 echen-98 echen-98 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@vijtrip2 vijtrip2

Labels

approved do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vijtrip2 @ack-bot @a-hilaly @RedbackThomson @echen-98