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

Added /healthz support #359

Merged
merged 3 commits into from
Oct 23, 2019
Merged

Added /healthz support #359

merged 3 commits into from
Oct 23, 2019

Conversation

banaag
Copy link
Collaborator

@banaag banaag commented Oct 22, 2019

Addresses #186

@banaag banaag requested review from twifkak and Gregable October 22, 2019 20:24
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@banaag banaag changed the base branch from releases to master October 22, 2019 20:25
@banaag
Copy link
Collaborator Author

banaag commented Oct 22, 2019

@googlebot

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@banaag
Copy link
Collaborator Author

banaag commented Oct 22, 2019

cc: @FranGM

Copy link
Member

@twifkak twifkak left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! Only nits.

packager/certcache/certcache.go Outdated Show resolved Hide resolved
packager/certcache/certcache.go Outdated Show resolved Hide resolved
packager/certcache/certcache.go Outdated Show resolved Hide resolved
packager/certcache/certcache.go Outdated Show resolved Hide resolved
packager/certcache/certcache.go Outdated Show resolved Hide resolved
packager/certcache/certcache.go Outdated Show resolved Hide resolved
packager/certcache/certcache.go Outdated Show resolved Hide resolved
packager/certcache/certcache.go Outdated Show resolved Hide resolved
packager/certcache/certcache_test.go Outdated Show resolved Hide resolved
packager/certcache/certcache.go Outdated Show resolved Hide resolved
@banaag banaag requested a review from twifkak October 23, 2019 17:29
func (this *CertCacheSuite) TestCertCacheIsHealthy() {
this.Assert().True(this.handler.IsHealthy())
this.Assert().Nil(this.handler.IsHealthy())
Copy link
Member

Choose a reason for hiding this comment

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

Optionally, use Assert().NoError(...) for I think a better failure msg.


this.Assert().False(this.handler.IsHealthy())
this.Assert().NotNil(this.handler.IsHealthy())
Copy link
Member

Choose a reason for hiding this comment

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

Ditto s/NotNil/Error

@banaag banaag merged commit 2e5f864 into ampproject:master Oct 23, 2019
@Gregable
Copy link
Member

LGTM

@banaag banaag added the acme All ACME related PRs label Feb 24, 2020
@banaag banaag deleted the healthz branch March 5, 2020 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acme All ACME related PRs cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants