Skip to content

Immutable fields advisory #70

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 1, 2021
Merged

Immutable fields advisory #70

merged 1 commit into from
Jun 1, 2021

Conversation

@nmvk
Copy link
Contributor

@nmvk nmvk commented May 20, 2021 •
edited
Loading

Add an advisory condition if an immutable field is modified.

  Conditions:
    Status:     False
    Type:       ACK.Terminal
    Status:     True
    Type:       ACK.ResourceSynced
    Status:     False
    Type:       ACK.Recoverable
    Message:    Immutable Spec fields have been modified : AtRestEncryptionEnabled,TransitEncryptionEnabled
    Status:     True
    Type:       ACK.Advisory
  Description:  Reinvent replication group

Clears the status upon fixing the field in the manifest

  Conditions:
    Status:     False
    Type:       ACK.Terminal
    Status:     True
    Type:       ACK.ResourceSynced
    Status:     False
    Type:       ACK.Recoverable
  Description:  Reinvent replication group

This commit allows one to specify immutable fields in generator.yaml.

      TransitEncryptionEnabled:
        is_immutable: true
      AtRestEncryptionEnabled:
        is_immutable: true

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 a-hilaly and jaypipes May 20, 2021 19:11
@nmvk nmvk mentioned this pull request May 20, 2021
Merged
@surajkota
Copy link
Contributor

surajkota commented May 20, 2021

how about making the message available in ACK.Recoverable itself?

because it looks like the controller will never be able to reach the desired state/spec so customer should revert the fields

jaypipes
jaypipes requested changes May 20, 2021
View reviewed changes
@jaypipes
Copy link
Collaborator

jaypipes commented May 21, 2021

how about making the message available in ACK.Recoverable itself?

because it looks like the controller will never be able to reach the desired state/spec so customer should revert the fields

ACK.ConditionTypeRecoverable was mainly for communicating with the user that an external dependency or failure happened and that the controller would be able to reconcile the resource if those external things were fixed. In this case, we want to indicate to the user that they've made a modification to the desired state that we're going to ignore and they should revert their changes to get rid of the message. Subtly different, IMHO.

jaypipes
jaypipes reviewed May 21, 2021
View reviewed changes
@nmvk nmvk requested a review from jaypipes May 27, 2021 08:07
RedbackThomson
RedbackThomson approved these changes May 27, 2021
View reviewed changes
ack-bot pushed a commit to aws-controllers-k8s/runtime that referenced this pull request May 31, 2021
@nmvk
Add new condition ACK.Advisory (#17)
8191f60 
New condition ACK.Advisory to set the condition
when any immutable field is modified.

This is needed for aws-controllers-k8s/code-generator#70
a-hilaly
a-hilaly approved these changes May 31, 2021
View reviewed changes
Copy link
Member

@a-hilaly a-hilaly left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM @nmvk 👍

Can you please change the PR description (Change ACKWarning to ACKAdvisory)?

@ack-bot will squash/commit using the PR TItle/description

templates/pkg/resource/sdk.go.tpl
Comment on lines 291 to 293
}
// handleImmutableFieldsChangedCondition validates the immutable fields and set appropriate condition
Copy link
Member

@a-hilaly a-hilaly May 31, 2021

Choose a reason for hiding this comment

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

nit: space between functions needed

@nmvk nmvk changed the title Immutable fields warning Immutable fields advisory May 31, 2021
@nmvk
Immutable fields advisory condition
019fd77 
Add an advisory condition if an immutable
field is modified.

This commit allows one to specify immutable
fields in generator.yaml.
@RedbackThomson
Copy link
Contributor

RedbackThomson commented Jun 1, 2021

/lgtm

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

ack-bot commented Jun 1, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, nmvk, 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,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 e625f74 into aws-controllers-k8s:main Jun 1, 2021
RedbackThomson pushed a commit to RedbackThomson/ack-runtime that referenced this pull request Jun 21, 2021
@nmvk
Add new condition ACK.Advisory (aws-controllers-k8s#17)
82c46d4 
New condition ACK.Advisory to set the condition
when any immutable field is modified.

This is needed for aws-controllers-k8s/code-generator#70
@echen-98 echen-98 mentioned this pull request Jun 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@a-hilaly a-hilaly a-hilaly approved these changes

@jaypipes jaypipes Awaiting requested review from jaypipes

+2 more reviewers

@RedbackThomson RedbackThomson RedbackThomson approved these changes

@vijtrip2 vijtrip2 vijtrip2 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@RedbackThomson RedbackThomson

Labels

approved lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@nmvk @surajkota @jaypipes @RedbackThomson @ack-bot @a-hilaly @vijtrip2