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.

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

Add Spec for EncryptionKeyRotation #65

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Add Spec for EncryptionKeyRotation #65

merged 1 commit into from
Jun 27, 2024

Conversation

black-dragon74
Copy link
Member

This patch adds specification for EncryptionKeyRotation operation of CSI addons.

@mergify mergify bot added the design Adds or updates an operation or service label May 23, 2024
@black-dragon74 black-dragon74 changed the title Add Spec for EncryptionKeyRotation [WIP] Add Spec for EncryptionKeyRotation May 23, 2024
@black-dragon74 black-dragon74 changed the title [WIP] Add Spec for EncryptionKeyRotation Add Spec for EncryptionKeyRotation Jun 10, 2024
@black-dragon74
Copy link
Member Author

@Madhu-1 @Rakshith-R requesting your reviews.

Regards

Rakshith-R
Rakshith-R reviewed Jun 10, 2024
View reviewed changes
encryptionkeyrotation/README.md Outdated Show resolved Hide resolved
encryptionkeyrotation/README.md Show resolved Hide resolved
nixpanic
nixpanic reviewed Jun 11, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
encryptionkeyrotation/README.md Outdated Show resolved Hide resolved
encryptionkeyrotation/README.md Outdated Show resolved Hide resolved
encryptionkeyrotation/README.md Show resolved Hide resolved
encryptionkeyrotation/README.md Show resolved Hide resolved
@nixpanic
Copy link
Contributor

There are a few failures in the CI jobs, please correct them as well.

Note that encryptionkeyrotation/encryptionkeyrotation.proto is generated from the README.md, and should not be modified manually.

@nixpanic
Copy link
Contributor

Also, code under lib/go is generated as well, it should be included in this PR too.

@black-dragon74
Copy link
Member Author

Also, code under lib/go is generated as well, it should be included in this PR too.

It is included in this PR. No?

@black-dragon74
Copy link
Member Author

Note that encryptionkeyrotation/encryptionkeyrotation.proto is generated from the README.md, and should not be modified manually.

I get the cause of the ci-failures. It has to do with me using different proto* set of tools for generating these files. There is a version mismatch. I will send a PR to add support for building on mac. Do we have to use versions as defined in makefile? If so, darwin builds for those specific versions are not available.

If we decide not to support mac, I can build it in docker and then send the PR. (Really hope that we support it though, will make things seamless, IMO)

Regards

@nixpanic
Copy link
Contributor

Also, code under lib/go is generated as well, it should be included in this PR too.

It is included in this PR. No?

Oh, yes, they are. I somehow missed them earlier.

@nixpanic
Copy link
Contributor

Note that encryptionkeyrotation/encryptionkeyrotation.proto is generated from the README.md, and should not be modified manually.

I get the cause of the ci-failures. It has to do with me using different proto* set of tools for generating these files. There is a version mismatch. I will send a PR to add support for building on mac. Do we have to use versions as defined in makefile? If so, darwin builds for those specific versions are not available.

Within a container (prefer Podman over Docker) should work for sure. I don't know to run that on a Mac though.

If we decide not to support mac, I can build it in docker and then send the PR. (Really hope that we support it though, will make things seamless, IMO)

Sure, that would be nice to have.

@black-dragon74
Copy link
Member Author

black-dragon74 commented Jun 13, 2024 •
edited
Loading

The linter is failing due to 403 (unable to access GH Status API), unrelated to this patch.

@nixpanic
Copy link
Contributor

The linter is failing due to 403 (unable to access GH Status API), unrelated to this patch.

I do not think the 403 error is critical, that is only failing to provide detailed status updates in the GitHub CI jobs list. If you check the actual failure, you see issues in the .proto linter:

image

@black-dragon74
Copy link
Member Author

you see issues in the .proto linter

We indent with 2 spaces in proto 🫨? Lesson learned.

Regards

Madhu-1
Madhu-1 reviewed Jun 13, 2024
View reviewed changes
identity/identity.proto Show resolved Hide resolved
identity/identity.proto Outdated Show resolved Hide resolved
encryptionkeyrotation/README.md Outdated Show resolved Hide resolved
identity/README.md Show resolved Hide resolved
@black-dragon74
Copy link
Member Author

@Madhu-1 can we get this merged?

Madhu-1
Madhu-1 reviewed Jun 20, 2024
View reviewed changes
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

small nits

encryptionkeyrotation/README.md Outdated Show resolved Hide resolved
identity/identity.proto Outdated Show resolved Hide resolved
Madhu-1
Madhu-1 reviewed Jun 20, 2024
View reviewed changes
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM, please squash the commits into 1.

@Rakshith-R Rakshith-R requested a review from Madhu-1 June 21, 2024 07:18
Rakshith-R
Rakshith-R reviewed Jun 21, 2024
View reviewed changes
Copy link
Member

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

Looks good to me.
@nixpanic can you please review again ?

Madhu-1
Madhu-1 previously approved these changes Jun 21, 2024
View reviewed changes
nixpanic
nixpanic reviewed Jun 21, 2024
View reviewed changes
encryptionkeyrotation/README.md Outdated Show resolved Hide resolved
encryptionkeyrotation/README.md Outdated Show resolved Hide resolved
encryptionkeyrotation/README.md Show resolved Hide resolved
@black-dragon74 black-dragon74 mentioned this pull request Jun 25, 2024
Open
@mergify mergify bot dismissed Madhu-1’s stale review June 25, 2024 12:55

Pull request has been modified.

nixpanic
nixpanic approved these changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

Thanks!

@black-dragon74
Copy link
Member Author

@Mergifyio refresh

@mergify Mergify
Copy link

mergify bot commented Jun 27, 2024

refresh

✅ Pull request refreshed

@Madhu-1
Copy link
Member

Madhu-1 commented Jun 27, 2024

@Mergifyio rebase

@black-dragon74 @Madhu-1
Add Spec for EncryptionKeyRotation
2cfec1d 
This patch adds specification for EncryptionKeyRotation
operation of CSI addons.

Signed-off-by: Niraj Yadav <niryadav@redhat.com>
@mergify Mergify
Copy link

mergify bot commented Jun 27, 2024

rebase

✅ Branch has been successfully rebased

@mergify mergify bot merged commit 0dd74d5 into csi-addons:main Jun 27, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rakshith-R Rakshith-R Rakshith-R left review comments

@nixpanic nixpanic nixpanic approved these changes

@Madhu-1 Madhu-1 Madhu-1 approved these changes

Assignees
No one assigned
Labels
design Adds or updates an operation or service
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@black-dragon74 @nixpanic @Madhu-1 @Rakshith-R