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

Key alias fix #950

Merged
merged 3 commits into from
Nov 17, 2021
Merged

Key alias fix #950

merged 3 commits into from
Nov 17, 2021

Conversation

muvaf
Copy link
Member

@muvaf muvaf commented Nov 17, 2021

Description of your changes

I merged #779 along with a few other PRs but apparently it broke lint action because of usage of a deprecated function from crossplane-runtime. Since we don't require PRs to be up-to-date, it slipped through the cracks.

When I tried to fix it, I realized that we can actually use external name annotation feature instead of spec.aliasName. But if we remove that field from spec, then we'd have all fields in ignore (total of two fields) and this causes generated for loops to be empty, hence code won't compile since the secondary value in for loop, i.e. _, elem := ... is unused. It's a bug in the code generator we need to fix and there isn't much workaround. Including TargetKeyId field didn't help since we have to ignore it because it's marked as required otherwise but we use ref & selector for that field. So, I ended up removing this from code-gen for now until the issue is fixed.

One additional thing I had to do was to assume that if Key is in PendingDeletion state, then we should treat it as deleted, otherwise it'll be stuck for days.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Manually together with KMS Key.

… empty spec which makes the for loops in conversion functions throw compile errors since there is nothing to process and variables end up unused

Signed-off-by: Muvaffak Onus <[email protected]>
…e otherwise it will not go away for days.

Signed-off-by: Muvaffak Onus <[email protected]>
@muvaf muvaf requested a review from negz November 17, 2021 00:27
@muvaf
Copy link
Member Author

muvaf commented Nov 17, 2021

Opened aws-controllers-k8s/community#1069 to address ignoring all fields in ReadMany type APIs.

@muvaf muvaf force-pushed the key-alias-fix branch 2 times, most recently from b668cd9 to 1231a51 Compare November 17, 2021 00:51
@muvaf muvaf mentioned this pull request Nov 17, 2021
9 tasks
Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

None of my comments are blockers.

@@ -14,8 +14,6 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

// Code generated by ack-generate. DO NOT EDIT.

Copy link
Member

Choose a reason for hiding this comment

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

Could be worth a note somewhere to explain why this resource looks like generated code, but "isn't".

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment to all those files pointing to description of this PR for details.

@muvaf muvaf merged commit e0fd313 into crossplane-contrib:master Nov 17, 2021
@muvaf muvaf deleted the key-alias-fix branch November 17, 2021 01:10
@github-actions
Copy link

Successfully created backport PR #951 for release-0.21.

tektondeploy pushed a commit to gtn3010/provider-aws that referenced this pull request Mar 12, 2024
…-and-arn-to-docdb-secret

added endpoint and arn to docdb cluster secret
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.

2 participants