Skip to content

feature: update observed generation on updateResource #731

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

Merged
merged 2 commits into from
Dec 8, 2021

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Dec 2, 2021

No description provided.

@@ -157,6 +157,9 @@ private R cloneResourceForErrorStatusHandlerIfNeeded(R resource, Context context
updatedCustomResource = updateStatusGenerationAware(updateControl.getResource());
} else if (updateControl.isUpdateResource()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't that if/else be simplified?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question, could try magically transform this, but what I tried to achieve here is just break it down based on the return cases from the controller. So it's obvious what happens on those cases. If you have any suggestion pls let me know.

Copy link
Contributor

@heesuk-ahn heesuk-ahn Dec 10, 2021

Choose a reason for hiding this comment

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

@csviri

If Enum is used to express Status in UpdateControl, it seems that if/else can be refactored 🤔 .

public class UpdateControl<T extends HasMetadata> extends BaseControl<UpdateControl<T>> {

  private final T resource;
  private final boolean updateStatus;
  private final boolean updateResource;
  private final UpdateControlStatusEnum controlStatusEnum; 
  ...
public enum UpdateControlStatusEnum {

  UPDATE_RESOURCE_AND_STATUS {
    @Override
    public <R extends HasMetadata> R toUpdatedResource(....) {
            return new UpdateResourceAndStatus(...);
        }
  },
  UPDATE_RESOURCE {
    @Override
    public <R extends HasMetadata> R toUpdatedResource(....) {
            return new UpdateResource(...);
        }
  },
  UPDATE_STATUS {
    @Override
    public <R extends HasMetadata> R toUpdatedResource(....) {
            return new UpdateStatus(...);
        }
  }
  
  // abstract method for business logic per status..
  // public abstract <R extends HasMetadata> R toUpdatedResource(...)
  
  // private static class UpdateResourceAndStatus implements UpdateCustomResource { .. }
  // private static class UpdateResource implements UpdateCustomResource { .. }
  // private static class UpdateStatus implements UpdateCustomResource { .. }
  // ...

Then in that class, you can use it like this:

class ReconciliationDispatcher<R extends HasMetadata> {
.
.
  private PostExecutionControl<R> reconcileExecution(ExecutionScope<R> executionScope,
      R resourceForExecution, R originalResource, Context context) {
   
    // REMOVED IF/ELSE...
    var updatedResource = updateControl.getControlStatusEnum.toUpdateResoruce(...);
    
    return createPostExecutionControl(updateResource, updateControl);
}

If refactoring is needed for this part, I think it can be improved in other issues and PullRequest. :)
Sorry for the late comment review!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be interested in seeing how this would work in a more fleshed-out PR, yes. Is that something you could work on, @heesuk-ahn?

Copy link
Contributor

Choose a reason for hiding this comment

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

@metacosm Yes, I will make an issue about this and work on it separately and upload the pr. :)

@csviri csviri force-pushed the update-observed-gen-on-res-update branch from e8ad63b to e180777 Compare December 8, 2021 11:45
@csviri csviri merged commit 5955ecd into main Dec 8, 2021
@csviri csviri deleted the update-observed-gen-on-res-update branch December 8, 2021 13:39
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.

Update observedGeneration also on UpdateControl.updateResource
3 participants