[Refactor] Move pollDeploymentStatus to resource.Deployment.CheckStatus#2896
Merged
tejal29 merged 2 commits intoGoogleContainerTools:masterfrom Sep 18, 2019
Merged
Conversation
tejal29
requested review from
balopat,
dgageot,
nkubala and
priyawadhwa
as code owners
tejal29
commented
Sep 17, 2019
| } | ||
|
|
||
| func TestGetRollOutStatus(t *testing.T) { | ||
| rolloutCmd := "kubectl --context kubecontext --namespace test rollout status deployment dep --watch=false" |
Contributor
Author
There was a problem hiding this comment.
Moved to TestCheckStatus in deployment_test.go
tejal29
changed the title
Codecov Report
|
tejal29
force-pushed
the
move_poll_deployment
branch
from
7aa6dbd to
3c57370
Compare
In this PR, - Move pollDeploymentStatus to resource.Deployment.CheckStatus - Add a new interface `Resource` which will be implemented by `Pod` and later. Current it is used for mocking `Deployment` in `pollResourceStatus` test. - Add a concept of retry and non-retry errors.
tejal29
force-pushed
the
move_poll_deployment
branch
from
3c57370 to
a7a1093
Compare
priyawadhwa
approved these changes
Sep 18, 2019
Contributor
priyawadhwa
left a comment
priyawadhwa
left a comment
There was a problem hiding this comment.
This looks really good to me! Just one suggestion.
tejal29
added a commit
to tejal29/skaffold
that referenced
this pull request
Sep 18, 2019
…al29/move_poll_deployment"" This reverts commit cf878dc.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #176
Fixes #2744
(Moved the test to use
mockResourcesinstead of deployments. Does not test the loop and always returns in first try)Should merge before :
Description
In this PR,
Resourcewhich will be implemented byPodand later.Current it is used for mocking
DeploymentinpollResourceStatustest.
Output Changes
n/a
Before
n/a
After
n/a
Next PRs.
next changes for #176,
resource.Baseand move all common functions for pod and deployment there.Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
examplesdir, please copy them tointegration/examplesintegration/examplesdir, should be tested in integration testReviewer Notes