Update if AlreadyExists #3202
Update if AlreadyExists #3202
Conversation
openshift-ci
bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
dtfranz
force-pushed
the
update-on-create-fail
branch
from
4418b49 to
95d3c37
Compare
|
Would it make sense to add the |
|
@tmshort every create call should already be setting that in the object payload. |
"should" It would mean that we guarantee adding the label, without having to remember everywhere... just a thought. Not necessary for this PR. |
|
@tmshort we spent quite a lot of time making sure it was present everywhere. If you have any examples of that not being the case, please open a PR. Locality of behavior is a nice property - if you're looking at code that creates an object, you can see what that object looks like without spelunking into hack-ey wrapper bits. Respectfully, I understand where you're coming from but I don't think we have any evidence that we missed any spots where the label should have been present in a create call and I'd prefer us to do targeted, specific changes rather than shots-in-the-dark-just-in-case-who-knows. |
dtfranz
force-pushed
the
update-on-create-fail
branch
from
95d3c37 to
3e45d2e
Compare
|
@dtfranz do we need to change code of CreateServiceAccount in serviceaccount.go to handle |
dtfranz
force-pushed
the
update-on-create-fail
branch
from
3e45d2e to
01496f7
Compare
|
@kuiwang02 I did this initially but adding that behavior universally to |
| @@ -734,6 +734,9 @@ func (c *ConfigMapUnpacker) ensureRole(cmRef *corev1.ObjectReference) (role *rba | |||
| if err != nil { | |||
| if apierrors.IsNotFound(err) { | |||
| role, err = c.client.RbacV1().Roles(fresh.GetNamespace()).Create(context.TODO(), fresh, metav1.CreateOptions{}) | |||
| if apierrors.IsAlreadyExists(err) { | |||
There was a problem hiding this comment.
bundle_unpacker.go does not have the wrapper client; I made the changes this way instead of spending the time to plumb the client in to keep the PR simpler and spend time on more important changes.
| @@ -150,7 +150,10 @@ func (r *OperatorConditionReconciler) ensureOperatorConditionRole(operatorCondit | |||
| if !apierrors.IsNotFound(err) { | |||
| return err | |||
| } | |||
| return r.Client.Create(context.TODO(), role) | |||
| err = r.Client.Create(context.TODO(), role) | |||
There was a problem hiding this comment.
As in bundler_unpacker.go, client wrapper is not available here.
Adds code to the client calls for Create to run an Update if the object already exists. This allows us to bypass issues where the object was not found in cache but already exists in the cluster. Signed-off-by: Daniel Franz <[email protected]>
dtfranz
force-pushed
the
update-on-create-fail
branch
from
01496f7 to
9b1be42
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| } else if err != nil { | ||
| return false, err | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
Nit, This could be:
return err == nil, err
For most, if not all, of the returns.
| return c.AppsV1().Deployments(dep.Namespace).Create(context.TODO(), dep, metav1.CreateOptions{}) | ||
| createdDep, err := c.AppsV1().Deployments(dep.Namespace).Create(context.TODO(), dep, metav1.CreateOptions{}) | ||
| if apierrors.IsAlreadyExists(err) { | ||
| updatedDep, _, err := c.UpdateDeployment(dep) |
There was a problem hiding this comment.
Three returns? Compared to the others that just have two? (Just whining)
|
Removed hold comment from above but I guess it needs a whole new comment. |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/approve |
a7b6658
In the same vein as operator-framework#3202, use update if the unpack job already exists but isn't cached Signed-off-by: kevinrizza <[email protected]>
In the same vein as operator-framework#3202, use update if the unpack job already exists but isn't cached Signed-off-by: kevinrizza <[email protected]>
In the same vein as #3202, use update if the unpack job already exists but isn't cached Signed-off-by: kevinrizza <[email protected]>
In the same vein as operator-framework/operator-lifecycle-manager#3202, use update if the unpack job already exists but isn't cached Signed-off-by: kevinrizza <[email protected]> Upstream-repository: operator-lifecycle-manager Upstream-commit: 47aaa6be4a951c6bd8be016f6610c5319adc6a47
In the same vein as operator-framework/operator-lifecycle-manager#3202, use update if the unpack job already exists but isn't cached Signed-off-by: kevinrizza <[email protected]> Upstream-repository: operator-lifecycle-manager Upstream-commit: 47aaa6be4a951c6bd8be016f6610c5319adc6a47
In the same vein as operator-framework/operator-lifecycle-manager#3202, use update if the unpack job already exists but isn't cached Signed-off-by: kevinrizza <[email protected]> Upstream-repository: operator-lifecycle-manager Upstream-commit: 47aaa6be4a951c6bd8be016f6610c5319adc6a47
In the same vein as operator-framework/operator-lifecycle-manager#3202, use update if the unpack job already exists but isn't cached Signed-off-by: kevinrizza <[email protected]> Upstream-repository: operator-lifecycle-manager Upstream-commit: 47aaa6be4a951c6bd8be016f6610c5319adc6a47
In the same vein as operator-framework/operator-lifecycle-manager#3202, use update if the unpack job already exists but isn't cached Signed-off-by: kevinrizza <[email protected]> Upstream-repository: operator-lifecycle-manager Upstream-commit: 47aaa6be4a951c6bd8be016f6610c5319adc6a47
Adds code to the client calls for Create to run an Update if the object already exists. This allows us to bypass issues where the object was not found in cache but already exists in the cluster.