Skip to content
This repository has been archived by the owner on Jul 30, 2021. It is now read-only.

Add support for configmap in checkpointer #320

Merged
merged 1 commit into from
Mar 6, 2017

Conversation

dhawal55
Copy link
Contributor

@dhawal55 dhawal55 commented Feb 18, 2017

Fixes #124

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 18, 2017
@ghost
Copy link

ghost commented Feb 18, 2017

Can one of the admins verify this patch?

@aaronlevy
Copy link
Contributor

ok to test

@dhawal55
Copy link
Contributor Author

Can someone publish details of the failed build (bootkube-etcd-dev) and let me know how i can fix it?

@aaronlevy
Copy link
Contributor

@dhawal55 the PR tests are a bit in a state of flux - and there are some flakes we're still working through (and not currently able to expose the logs publicly - but it's on roadmap). Sorry for the delay on this (and thank you for the contribution!). I'll try and take a look at this/test early next week.

@aaronlevy aaronlevy added kind/feature Categorizes issue or PR as related to a new feature. needs review labels Feb 24, 2017
Copy link
Contributor

@aaronlevy aaronlevy left a comment

Choose a reason for hiding this comment

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

One really small comment.

cp, err = checkpointConfigMapVolumes(client, cp)
if err != nil {
//TODO(aaron): This can end up spamming logs at times when api-server is unavailable. To reduce spam
// we could only log error if api-server can't be contacted and existing secret doesn't exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/existing secret/existing configmap/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, good catch, it's a copy paste mistake. I will correct it

@aaronlevy
Copy link
Contributor

Overall this looks good, thank you for contributing this!

I'll run some tests against it, but assuming those pass - we should be able to get this merged soon.

@ghost
Copy link

ghost commented Feb 28, 2017

Can one of the admins verify this patch?

@aaronlevy
Copy link
Contributor

rktbot run tests

@aaronlevy
Copy link
Contributor

The PR tests might actually be down right now (due to S3 outages).

@peebs
Copy link

peebs commented Mar 1, 2017

rktbot run tests

@aaronlevy
Copy link
Contributor

aaronlevy commented Mar 3, 2017

@dhawal55 would you also mind squashing your commits?

@aaronlevy
Copy link
Contributor

@dhawal55 also looks like there are some conflicts that need to be resolved

@peebs
Copy link

peebs commented Mar 3, 2017

rktbot run checkpointer tests

@dhawal55
Copy link
Contributor Author

dhawal55 commented Mar 5, 2017

Squashed the commits and fixed merge issues

@aaronlevy aaronlevy added this to the v0.3.10 milestone Mar 6, 2017
@aaronlevy
Copy link
Contributor

rktbot run checkpointer tests

@aaronlevy
Copy link
Contributor

I manually tested this and it worked. The checkpointer-dev test I believe has an unrelated issue.

@pbx0 when you get a chance can you look into the failed test. It looks like re-running the etcd-scale test the second time failed.

Copy link
Contributor

@aaronlevy aaronlevy left a comment

Choose a reason for hiding this comment

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

lgtm

@aaronlevy aaronlevy merged commit 417b8f7 into kubernetes-retired:master Mar 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants