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

fix(ci) support certmanager 1.4 #697

Merged
merged 1 commit into from
Jul 13, 2021

Conversation

bitsf
Copy link
Collaborator

@bitsf bitsf commented Jul 5, 2021

close #693

@bitsf bitsf marked this pull request as draft July 5, 2021 14:43
@bitsf bitsf force-pushed the fix_certmanager_1.4 branch 3 times, most recently from 6435107 to b410548 Compare July 6, 2021 01:53
Signed-off-by: Ziming Zhang <zziming@vmware.com>
@bitsf bitsf marked this pull request as ready for review July 6, 2021 03:02
@@ -221,6 +221,8 @@ jobs:
exit 1
fi

sleep 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sleep 10
time kubectl wait -n ${operatorNamespace} pod --selector=control-plane=harbor-operator --timeout=300s

It seems possible to use this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

前面已经 wait deployment 了,此处怀疑是 harbor-operator 即便都已经部署成功了,webhook 也需要点时间才能生效,所以简单的 sleep

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that easy to understand the response, but I agree with @cndoit18 's suggestion

Copy link
Collaborator

Choose a reason for hiding this comment

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

前面已经 wait deployment 了,此处怀疑是 harbor-operator 即便都已经部署成功了,webhook 也需要点时间才能生效,所以简单的 sleep

@bitsf next time, you should leave an English message.

And will you do change for that part?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This sleep 10 is just to workaround CI failed sometimes recently, because webhook seems not started even we already using kubectl wait deployment

@@ -221,6 +221,8 @@ jobs:
exit 1
fi

sleep 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that easy to understand the response, but I agree with @cndoit18 's suggestion

@@ -82,7 +82,7 @@ func (r *Reconciler) GetNotarySignerCertificateAuthority(ctx context.Context, ha
Duration: &metav1.Duration{
Duration: duration,
},
CommonName: r.NormalizeName(ctx, harbor.GetName(), controllers.NotarySigner.String()),
CommonName: r.NormalizeName(ctx, harbor.GetName(), controllers.NotarySigner.String(), "CA"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this fix cert-manager v1.4 compatibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cert-manger 1.4 doesn't allow using same CN between Cert and CA, so I added a suffix to distinguish them

Copy link
Collaborator

@cndoit18 cndoit18 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@steven-zou steven-zou left a comment

Choose a reason for hiding this comment

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

LGTM

@steven-zou steven-zou merged commit 599e7a1 into goharbor:master Jul 13, 2021
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.

cert-manager 1.4.0 not support
5 participants