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

Filter empty length key diffs in Storage Account resource #816

Merged

Conversation

mergenci
Copy link
Collaborator

@mergenci mergenci commented Sep 13, 2024

Description of your changes

Fixes #683, #807

I have:

  • Read and followed Crossplane'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

See minimally-working example manifest's Uptest result.

Example manifest, which is introduced by this PR, won't reproduce the linked issues above. Therefore, apply the following manifest to reproduce the error:

apiVersion: storage.azure.upbound.io/v1beta1
kind: Account
metadata:
  name: issue816account
spec:
  deletionPolicy: Delete
  forProvider:
    accountKind: BlockBlobStorage
    accountReplicationType: ZRS
    accountTier: Premium
    allowNestedItemsToBePublic: false
    crossTenantReplicationEnabled: true
    enableHttpsTrafficOnly: true
    infrastructureEncryptionEnabled: true
    isHnsEnabled: true
    localUserEnabled: true
    location: East US
    minTlsVersion: TLS1_2
    networkRules:
      - defaultAction: Deny
        ipRules:
        - "0.0.0.0/0"
    nfsv3Enabled: true
    publicNetworkAccessEnabled: true
    queueEncryptionKeyType: Service
    resourceGroupName: example-resources-test
    sharedAccessKeyEnabled: true
    tableEncryptionKeyType: Service

---

apiVersion: azure.upbound.io/v1beta1
kind: ResourceGroup
metadata:
  name: example-resources-test
spec:
  forProvider:
    location: "West Europe"
    tags:
      provisioner: crossplane

@mergenci
Copy link
Collaborator Author

/test-examples="examples/storage/v1beta1/account.yaml"

@mergenci
Copy link
Collaborator Author

@nitang22, Would you be willing to test this PR in your local environment with your resource manifest? The manifest I prepared for Uptest fails and I don't have a local environment handy to test.

@nitang22
Copy link

@mergenci Sure, however, I will need some guidance for the test, is there any documentation on how I can compile your code ?

@mergenci
Copy link
Collaborator Author

@nitang22, I don't think we have an up-to-date documentation for local development. So, I found it easier to set up my local Azure environment 🙂 The PR fixes the issues. I need more time to work on an example manifest for automated tests.

Until we release a new version, you can use the following monolith image to test in your environment: index.docker.io/mergenci/provider-azure:v1.6.0-rc-pr-816-04b137b

I published a monolith image, which includes all family providers in one package, because installing a family provider requires installing dependencies, which is difficult with a third-party repository. Monolith provider doesn't have any dependencies, so you can install it by itself.

@mergenci
Copy link
Collaborator Author

/test-examples="examples/storage/v1beta1/account.yaml"

@stevendborrelli
Copy link
Contributor

FYI, this PR seems to fix the issue. Can we get this merged?

Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

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

Thank you @mergenci, after fixing gocyclo failure or ignoring it like here, we can move PR forward.

Signed-off-by: Cem Mergenci <cmergenci@gmail.com>
Signed-off-by: Cem Mergenci <cmergenci@gmail.com>
@mergenci
Copy link
Collaborator Author

Thanks for letting me know @turkenf. I acted out of my prejudice against linter usually failing and didn't check why it failed 😒 Done.

Copy link
Collaborator

@turkenf turkenf left a comment

Choose a reason for hiding this comment

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

Thank you @mergenci, LGTM.

@mergenci mergenci merged commit 7be17a7 into crossplane-contrib:main Sep 25, 2024
11 checks passed
@mergenci mergenci deleted the storage-account-empty-diffs branch September 25, 2024 07:40
Copy link

Successfully created backport PR #826 for release-1.6.

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

Successfully merging this pull request may close these issues.

[Bug]: Storage account always showing diff detected
4 participants