[WIP] [NE-72] Handle updates to ClusterIngress and update generated ingress… #89
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ramr If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@ramr: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| // ensureRouterAsset ensures the expected router asset exists and returns the | ||
| // current asset information. | ||
| func (r *reconciler) ensureRouterAsset(key types.NamespacedName, obj runtime.Object) (runtime.Object, error) { | ||
| err := r.Client.Get(context.TODO(), key, obj) |
There was a problem hiding this comment.
This is mutating the input which is surprising (no docs). Seems like a generic dynamic client concern, not something scoped to just router assets. Maybe extend the Client interface or something?
Also your deep copy additions reminded me that we're probably not being very safe generally in terms of mutating potentially cached items from our client rather than doing copies everywhere...
ironcladlou
left a comment
ironcladlou
left a comment
There was a problem hiding this comment.
Looking pretty good so far. I'm really afraid of bundling too many state change handlers into this PR which could be incrementally introduced. What if we tried establishing the patterns with the easy ones first (like the cluster and namespace stuff) and then moved on to successively complex cases in followups?
| } | ||
|
|
||
| current := cr.DeepCopy() | ||
| key := types.NamespacedName{Namespace: cr.Namespace, Name: cr.Name} |
There was a problem hiding this comment.
Cluster role isn't namespaced
| } else if !errors.IsAlreadyExists(err) { | ||
| return fmt.Errorf("failed to create router cluster role %s: %v", cr.Name, err) | ||
| } | ||
| if err := r.ensureRouterClusterRole(); err != nil { |
There was a problem hiding this comment.
This isn't namespaced, would it make sense to move it out of ensureRouterNamespace?
| // TODO: Should we check for Secrets, ImagePullSecrets and | ||
| // AutomountServiceAccountToken - we don't really use those. | ||
| // TODO: should we check for label/annotation changes or ignore them? | ||
| modified := !reflect.DeepEqual(current.Labels, sa.Labels) || !reflect.DeepEqual(current.Annotations, sa.Annotations) |
There was a problem hiding this comment.
This is an annoying one since the struct has no Spec to reflectively compare... is there some util in library-go or elsewhere for a generic "has anything mutable changed" comparisons?
|
@ironcladlou thanks for the comments ... will do the suggested fixes and break this up into 2 PRs as you suggested over the next few days (one for the smaller change to cluster+namespace and the other for the rest). Keeping this around for reference for now. |
|
Closing this out in favor of #94 which only updates a subset of the generated resources. |
3 participants
… assets.
Ref: https://jira.coreos.com/browse/NE-72
This is still work in progress - will add tests as well but first wanted to confirm this works
for you @ironcladlou PTAL Thx