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 buggy ownership logic for Pods in Deployment awaiter #311

Merged
merged 4 commits into from
Dec 4, 2018

Conversation

lblackstone
Copy link
Member

The Deployment awaiter was erroneously overwriting aggregated
Pod errors if they had the same name due to buggy ownership checking.
Fixed the ownership logic and the relevant tests.

The Deployment awaiter was erroneously overwriting aggregated
Pod errors if they had the same name due to buggy ownership checking.
Fixed the ownership logic and the relevant tests.
@@ -459,7 +458,7 @@ func (dia *deploymentInitAwaiter) checkReplicaSetStatus() {
// we would now have fewer replicas than we had requested in the `Deployment` we last submitted
// when we last ran `pulumi up`.
rawSpecReplicas, specReplicasExists := openapi.Pluck(rs.Object, "spec", "replicas")
specReplicas, _ := rawSpecReplicas.(float64)
specReplicas, _ := rawSpecReplicas.(int64)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, looks like this is actually an *int32? Have we double checked the type with reflect.TypeOf, just to be safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just checked and it said int64

Copy link
Contributor

@hausdorff hausdorff left a comment

Choose a reason for hiding this comment

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

I think this is ready to go, I'd go ahead and bump to 60 minutes for now though... it's a huge pain to clean up if CI times out.

Should we follow up with a bug to just make the integration tests output to std out, as well? This should "solve" the problem.

@lblackstone
Copy link
Member Author

Bumped the timeout. Yeah, let's output to stdout if possible.

@lblackstone lblackstone merged commit 8b1457a into master Dec 4, 2018
@pulumi-bot pulumi-bot deleted the lblackstone/fix-deployment-pod-ownership branch December 4, 2018 18:51
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.

None yet

2 participants