-
Notifications
You must be signed in to change notification settings - Fork 115
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
Add await support for networking.k8s.io/v1 variant of ingress #1795
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
2 similar comments
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
1 similar comment
Does the PR have any schema changes?Looking good! No breaking changes found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to me broadly - although the actual logic is beyond my skill level. I trust Vivek and Levi on this one.
assert.Equal(t, tokens.Type("kubernetes:core/v1:Service"), redisMasterService.URN.Type()) | ||
name, _ = openapi.Pluck(redisMasterService.Outputs, "metadata", "name") | ||
assert.Equal(t, "redis-master", name) | ||
status, _ = openapi.Pluck(redisMasterService.Outputs, "spec", "clusterIP") | ||
assert.True(t, len(status.(string)) > 1) | ||
|
||
// Verify redis-slave service. | ||
redisSlaveService := stackInfo.Deployment.Resources[6] | ||
redisSlaveService := stackInfo.Deployment.Resources[9] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Index-based lookups look somewhat unreliable, I wonder if we could change to name-based?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was copy pasta from a previous test I based off of. Its not material to the ingress behavior under test here so removed the resource assertions entirely.
Does the PR have any schema changes?Looking good! No breaking changes found. |
3 similar comments
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
cc84266
to
f133163
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
Proposed changes
It appears the core issue with the lack of correct await on ingress objects was because of missing support for networking.k8s.io/v1 variants of the ingress object. I considered trying to canonicalize against the preferred version of the api server, but things get complicated quickly when taking upgrades into account. The approach here allows us to support all of the known versions for ingress by adjusting the await logic to each variant's API shape.
Related issues (optional)
#1649 #1498