Skip to content

Fix for issue 505 - #690

Merged
turkenh merged 1 commit into
crossplane-contrib:mainfrom
svscheg:issue505
Jun 12, 2023
Merged

Fix for issue 505#690
turkenh merged 1 commit into
crossplane-contrib:mainfrom
svscheg:issue505

Conversation

@svscheg

@svscheg svscheg commented May 4, 2023

Copy link
Copy Markdown
Contributor

Description of your changes

Fix for issue 505

  1. Change "aws_ses_domain_identity": config.config.IdentifierFromProvider, to "aws_ses_domain_identity": config.ParameterAsIdentifier("domain"),
  2. Add to the aws_ses_domain_identity and aws_ses_domain_dkim crossplane.io/external-name value

Fixes #505

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Manually

@svscheg
svscheg force-pushed the issue505 branch 2 times, most recently from f267e7c to 784b007 Compare May 4, 2023 12:28
@sergenyalcin

Copy link
Copy Markdown
Collaborator

I think this PR can be fix #505. In theory, domain is an optional field and we will not have a breaking change here. However, we will have a behavioral change. I would prefer to switch aws_ses_domain_dkim": config.IdentifierFromProvider instead of changing the aws_ses_domain_identity configuration.

I want to also hear @ulucinar's idea on this.

@turkenf

turkenf commented Jun 8, 2023

Copy link
Copy Markdown
Collaborator

/test-examples="examples/ses/domainidentity.yaml"

@turkenh

turkenh commented Jun 8, 2023

Copy link
Copy Markdown
Contributor

I think this PR can be fix #505. In theory, domain is an optional field and we will not have a breaking change here. However, we will have a behavioral change. I would prefer to switch aws_ses_domain_dkim": config.IdentifierFromProvider instead of changing the aws_ses_domain_identity configuration.

I want to also hear @ulucinar's idea on this.

Just left a comment on the issue regarding this: #505 (comment)

@svscheg I believe we should make sure that this is not a breaking change by:

  1. Install the existing provider version.
  2. Create a DomainIdentity resource and make sure it is ready & synced.
  3. Upgrade to a provider built from this PR (see Publishing Provider packages to Dockerhub (for testing or local development) crossplane/crossplane#3817)
  4. Make sure provider is upgraded successfully.
  5. Make sure the DomainIdentity resource stays ready & synced without any issues.

@turkenh

turkenh commented Jun 8, 2023

Copy link
Copy Markdown
Contributor

@svscheg could you also remove the Draft in the title once this PR is ready for review/merge.

@turkenf turkenf changed the title [DRAFT] Fix for issue 505 Fix for issue 505 Jun 12, 2023
@turkenf

turkenf commented Jun 12, 2023

Copy link
Copy Markdown
Collaborator

@turkenh I couldn't test with the method you suggested above. (I couldn’t build provider package from this PR, procedure here didn’t work.)
I have successfully tested it as described below:

First, I installed provider-aws v0.35.0 in the marketplace and successfully created the ses.DomainIdentity resource containing the domain: "example.com"spec:

  deletionPolicy: Delete
  forProvider:
    domain: example.com
    region: us-west-1
  managementPolicy: FullControl
  providerConfigRef:
    name: default parameter. The spec of the resource created as expected:

Secondly, I stopped the provider-aws v0.35.0 running in my cluster and applied the CRDs through this PR. After running make run, the resource remains READY=True, SYNCED=True state:

spec:
  deletionPolicy: Delete
  forProvider:
    region: us-west-1
  managementPolicy: FullControl
  providerConfigRef:
    name: default
status:
  atProvider:
    arn: arn:aws:ses:us-west-1:153891904029:identity/example.com
    id: example.com
    verificationToken: Ms0KtRPQtrSrI0ZrOX23c06oPaxL6sbpkyZWLVG9hjk=
  conditions:
  - lastTransitionTime: "2023-06-12T09:09:10Z"
    reason: Available
    status: "True"
    type: Ready
  - lastTransitionTime: "2023-06-12T09:09:02Z"
    reason: ReconcileSuccess
    status: "True"
    type: Synced
  - lastTransitionTime: "2023-06-12T09:09:06Z"
    reason: Success
    status: "True"
    type: LastAsyncOperation
  - lastTransitionTime: "2023-06-12T09:09:06Z"
    reason: Finished
    status: "True"
    type: AsyncOperation

@turkenh

turkenh commented Jun 12, 2023

Copy link
Copy Markdown
Contributor

@turkenh I couldn't test with the method you suggested above. (I couldn’t build provider package from this PR, procedure here didn’t work.)

Thanks @turkenf for testing. The procedure you followed already covers potential problems but it would still be valuable to see the package upgrade works properly. Could you retest with the RC image published after merging this PR?

@turkenh
turkenh merged commit c0ff895 into crossplane-contrib:main Jun 12, 2023
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.

ses: DomainDKIM CRD domain key is missing

4 participants