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

Check LateInitialize management policy in Plugin Framework external client #350

Conversation

mergenci
Copy link
Member

@mergenci mergenci commented Feb 21, 2024

Description of your changes

  1. Before calling LateInitialize(), make sure that spec.managementPolicies include LateInitialize policy.
  2. Extend move RequiresReplace check of plan to Update time instead of Observe & suppress diffs with only computed attributes #341 to handle optional attributes, in addition to computed ones.

I have:

  • Read and followed Upjet's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

I manually tested using an AWS Security Group Ingress Rule with the following configuration:

apiVersion: ec2.aws.upbound.io/v1beta1
kind: VPC
metadata:
  annotations:
    meta.upbound.io/example-id: ec2/v1beta1/securitygroupingressrule
  name: test-pr-350-vpc
  labels:
    testing.upbound.io/example-name: test-pr-350-vpc
spec:
  forProvider:
    region: us-west-1
    cidrBlock: 172.16.0.0/16
    tags:
      Name: TestCemVPC

---
apiVersion: ec2.aws.upbound.io/v1beta1
kind: SecurityGroup
metadata:
  annotations:
    meta.upbound.io/example-id: ec2/v1beta1/securitygroupingressrule
  name: test-pr-350-securitygroup
  labels:
    testing.upbound.io/example-name: test-pr-350-securitygroup
spec:
  forProvider:
    region: us-west-1
    vpcIdSelector:
      matchLabels:
        testing.upbound.io/example-name: test-pr-350-vpc

---
apiVersion: ec2.aws.upbound.io/v1beta1
kind: SecurityGroupIngressRule
metadata:
  name: test-pr-350-securitygroupingressrule
spec:
  managementPolicies: ["Create", "Observe", "Update", "Delete", "LateInitialize"]
  initProvider:
    tags:
      foo: barInitProvider
  forProvider:
    cidrIpv4: 10.0.0.0/8
    fromPort: 8080
    ipProtocol: tcp
    region: us-west-1
    securityGroupIdRef:
      name: test-pr-350-securitygroup
    toPort: 8081

description field is optional and is not specified in the configuration above. After the resource is created, set its description from AWS Console and trigger a reconciliation. Check that spec.forProvider.description and status.atProvider.description are set after the reconciliation is finished.

References

This PR is a follow-up to #343.

@mergenci mergenci marked this pull request as draft February 21, 2024 07:28
@mergenci
Copy link
Member Author

@erhancagirici, do you have any concerns over this modification?

@mergenci
Copy link
Member Author

@ulucinar, can you please add backport release-1.2 label to the PR? Looks like I don't have privileges to do so.

@mergenci mergenci marked this pull request as ready for review February 21, 2024 07:33
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @mergenci, let's also make sure these changes are consumed in the upbound/provider-aws v1.1 branch.

pkg/controller/external_tfpluginfw.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@erhancagirici erhancagirici left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @mergenci

@mergenci mergenci force-pushed the external-client-check-lateinitialize-management-policy branch from 5694725 to af31423 Compare February 21, 2024 12:57
@mergenci mergenci merged commit eaa1e9a into crossplane:main Feb 21, 2024
7 checks passed
@mergenci mergenci deleted the external-client-check-lateinitialize-management-policy branch February 21, 2024 13:06
Copy link

Validation Failed: "Could not add requested reviewers to pull request."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants