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 Deployment await logic for old API schema #523

Merged
merged 3 commits into from
Apr 9, 2019

Conversation

lblackstone
Copy link
Member

Resources created with the old extensions/v1beta1:Deployment schema
do not include two status fields that the await logic was expecting. Add
an exception for this API version to make those checks pass.

Fixes #519

Resources created with the old extensions/v1beta1:Deployment schema
do not include two status fields that the await logic was expecting. Add
an exception for this API version to make those checks pass.
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.

This is ok to unblock people, but it looks to me that we're (1) not checking that the new RS has become available, and (2) not checking the Progressing status (which doesn't exist) -- in my estimation this removes entirely the await logic for Deployments declared with version extensions/v1beta1.

Can we file a follow-up issue to try to do something smarter here?

Also, note that we cannot simply use the Kubernetes schema auto-translation -- if declared as extensions/v1beta1 even kubectl get'ing the .status field does not actually cause these fields to appear. This sucks, and effectively means that we'll have to have a parallel await logic path for this specific API version. Worth nothing that in the issue.

@hausdorff
Copy link
Contributor

One other thing -- does the bug described in #519 also happen in apps/v1beta1 or apps/v1beta2? If we haven't, we should double check just to make sure.

@lblackstone
Copy link
Member Author

lblackstone commented Apr 9, 2019

One other thing -- does the bug described in #519 also happen in apps/v1beta1 or apps/v1beta2? If we haven't, we should double check just to make sure.

Those both work as usual; no changes required.

@lblackstone lblackstone merged commit f9c89f2 into master Apr 9, 2019
@pulumi-bot pulumi-bot deleted the lblackstone/deployment-await-fix branch April 9, 2019 22:59
lblackstone added a commit that referenced this pull request Sep 11, 2019
The extensions/v1beta1 version of Deployment uses a different
code path because it doesn't include all of the fields we use to
check readiness on newer apiVersions. This logic was updated
in #523, but introduced a different bug in the process. The logic
should now correctly check for readiness across all Deployment
apiVersions.
lblackstone added a commit that referenced this pull request Sep 11, 2019
The extensions/v1beta1 version of Deployment uses a different
code path because it doesn't include all of the fields we use to
check readiness on newer apiVersions. This logic was updated
in #523, but introduced a different bug in the process. The logic
should now correctly check for readiness across all Deployment
apiVersions.
lblackstone added a commit that referenced this pull request Sep 11, 2019
The extensions/v1beta1 version of Deployment uses a different
code path because it doesn't include all of the fields we use to
check readiness on newer apiVersions. This logic was updated
in #523, but introduced a different bug in the process. The logic
should now correctly check for readiness across all Deployment
apiVersions.
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