Skip to content

Valid pem format to the CRDs webhooks instead empty placeholder#1522

Merged
fedepaol merged 2 commits intometallb:mainfrom
cyclinder:fix_invalid_caBundle
Jul 19, 2022
Merged

Valid pem format to the CRDs webhooks instead empty placeholder#1522
fedepaol merged 2 commits intometallb:mainfrom
cyclinder:fix_invalid_caBundle

Conversation

@cyclinder
Copy link
Contributor

@cyclinder cyclinder commented Jul 19, 2022

We need to have a valid format and not only the placeholder, as the apiserver will refuse deletion with "unable to load root certificates: unable to parse bytes as PEM block"

Fixed #1501
Fixed #1521

@fedepaol
Copy link
Member

We are trying to have the commit messages as per https://cbea.ms/git-commit/

Can you change the commit with something like:

Add a valid pem format to the CRDs webhooks instead of the empty placeholder

We need to have a valid format and not only the placeholder, as the apiserver will refuse deletion with "unable to load root certificates: unable to parse bytes as PEM block" ?

@fedepaol
Copy link
Member

Also, can you add another commit to track this change to the release notes here

?

@fedepaol
Copy link
Member

Also also, can you change the PR description with the same content as the commit message?

@cyclinder cyclinder changed the title fix the incorrect format of caBundle that cause can't list crds Add a valid pem format to the CRDs webhooks instead of the empty placeholder Jul 19, 2022
@cyclinder cyclinder force-pushed the fix_invalid_caBundle branch from aa303ab to ab16b95 Compare July 19, 2022 10:32
@cyclinder
Copy link
Contributor Author

Thanks @fedepaol

@cyclinder
Copy link
Contributor Author

I found "First-time contributors need a maintainer to approve running workflows," Could you enable CI for the PR ?

@fedepaol
Copy link
Member

We are trying to have the commit messages as per https://cbea.ms/git-commit/

Can you change the commit with something like:

Add a valid pem format to the CRDs webhooks instead of the empty placeholder

We need to have a valid format and not only the placeholder, as the apiserver will refuse deletion with "unable to load root certificates: unable to parse bytes as PEM block" ?

Sorry I wasn't clear. What I meant above was also to change the commit body with the second part here ^^^^ :

We need to have a valid format and not only the placeholder, as the apiserver will refuse deletion with "unable to load root certificates: unable to parse bytes as PEM block

Also, the commit title is too long and gets cut, please try to shorten it (Valid pem format to the CRDs webhooks instead empty placeholder should work)

@cyclinder cyclinder force-pushed the fix_invalid_caBundle branch from ab16b95 to 0fd895b Compare July 19, 2022 10:59
@cyclinder
Copy link
Contributor Author

We are trying to have the commit messages as per https://cbea.ms/git-commit/
Can you change the commit with something like:
Add a valid pem format to the CRDs webhooks instead of the empty placeholder
We need to have a valid format and not only the placeholder, as the apiserver will refuse deletion with "unable to load root certificates: unable to parse bytes as PEM block" ?

Sorry I wasn't clear. What I meant above was also to change the commit body with the second part here ^^^^ :

We need to have a valid format and not only the placeholder, as the apiserver will refuse deletion with "unable to load root certificates: unable to parse bytes as PEM block

Also, the commit title is too long and gets cut, please try to shorten it (Valid pem format to the CRDs webhooks instead empty placeholder should work)

If my understand is right:

PR title: "Valid pem format to the CRDs webhooks instead empty placeholder should work",
First commit message: "Valid pem format to the CRDs webhooks instead empty placeholder should work",
Second commit message: "Release notes: Add a valid pem format to the CRDs webhooks instead of the empty placeholder"

Is this ok?

@fedepaol
Copy link
Member

We are trying to have the commit messages as per https://cbea.ms/git-commit/
Can you change the commit with something like:
Add a valid pem format to the CRDs webhooks instead of the empty placeholder
We need to have a valid format and not only the placeholder, as the apiserver will refuse deletion with "unable to load root certificates: unable to parse bytes as PEM block" ?

Sorry I wasn't clear. What I meant above was also to change the commit body with the second part here ^^^^ :

We need to have a valid format and not only the placeholder, as the apiserver will refuse deletion with "unable to load root certificates: unable to parse bytes as PEM block

Also, the commit title is too long and gets cut, please try to shorten it (Valid pem format to the CRDs webhooks instead empty placeholder should work)

If my understand is right:

PR title: "Valid pem format to the CRDs webhooks instead empty placeholder should work",
First commit message: "Valid pem format to the CRDs webhooks instead empty placeholder should work",
Second commit message: "Release notes: Add a valid pem format to the CRDs webhooks instead of the empty placeholder"

Is this ok?

PR title: "Valid pem format to the CRDs webhooks instead empty placeholder",
First commit message: "Valid pem format to the CRDs webhooks instead empty placeholder",
Second commit message: "Release notes: mention the cabundle placeholder fix"

Also: add the suggested body:

We need to have a valid format and not only the placeholder, as the apiserver will refuse deletion with "unable to load root certificates: unable to parse bytes as PEM block.

to the first commit

@cyclinder cyclinder force-pushed the fix_invalid_caBundle branch from 0fd895b to bbf0efd Compare July 19, 2022 11:23
@cyclinder cyclinder changed the title Add a valid pem format to the CRDs webhooks instead of the empty placeholder Valid pem format to the CRDs webhooks instead empty placeholder Jul 19, 2022
@cyclinder cyclinder force-pushed the fix_invalid_caBundle branch from bbf0efd to cc6a90e Compare July 19, 2022 11:34
@cyclinder
Copy link
Contributor Author

cyclinder commented Jul 19, 2022

@fedepaol Thanks, Updated. Is the Run inv generatemanifests required?

root@master:~/metallb# inv generatemanifests
/bin/bash: line 1: controller-gen: command not found

@fedepaol
Copy link
Member

@fedepaol Thanks, Updated. Is the Run inv generatemanifests required?

root@master:~/metallb# inv generatemanifests
/bin/bash: line 1: controller-gen: command not found

nope, I see you changed the manifests manually. CI will complain if they are not consistent with the generated result.

@fedepaol
Copy link
Member

The requirements for dev are described here https://metallb.universe.tf/community/#required-software

@fedepaol
Copy link
Member

@cyclinder so, CI failed for some difference between the generated manifests under https://github.com/metallb/metallb/tree/main/config/manifests which are generated from the kustomize configuration (including https://github.com/metallb/metallb/blob/main/config/crd/crd-conversion-patch.yaml which you changed).
In order to get it straight, you need to download and install invoke (https://www.pyinvoke.org/) and run inv generatemanifests as CI suggests (please amend the first commit and not add a new one)

@cyclinder
Copy link
Contributor Author

OK, I am very sorry for not solving the problem in a short time, now I will solve it

@fedepaol
Copy link
Member

OK, I am very sorry for not solving the problem in a short time, now I will solve it

no problem, and thanks for the help!

@fedepaol
Copy link
Member

Also, come to slack if you need help with that

We need to have a valid format and not only the placeholder, as the apiserver will refuse deletion with "unable to load root certificates: unable to parse bytes as PEM block"
Signed-off-by: cyclinder <qifeng.guo@daocloud.io>
@cyclinder cyclinder force-pushed the fix_invalid_caBundle branch from cc6a90e to 2609852 Compare July 19, 2022 14:03
@fedepaol
Copy link
Member

LGTM, thanks!

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

Labels

None yet

Projects

None yet

2 participants