Skip to content

How to selectively define the fields to reconcile with DependentResources #1474

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

Closed
andreaTP opened this issue Sep 16, 2022 · 8 comments · Fixed by #1746
Closed

How to selectively define the fields to reconcile with DependentResources #1474

andreaTP opened this issue Sep 16, 2022 · 8 comments · Fixed by #1746
Assignees
Milestone

Comments

@andreaTP
Copy link
Collaborator

Bug Report

Defining a DependenResource of type e.g. Job is slightly problematic at a first attempt.
The reason is that the Job .spec.template field is immutable and any update operation slightly changing it result in an error.

Here I'm tentatively disabling all the update methods:
https://github.com/andreaTP/bink8s/blob/0177a66e4b31225c0b5a91e5694e80a3d3127df4/src/main/java/io/bink8s/controller/workflow/ImageBuildJobDependentResource.java#L85-L97

I'm not sure what would be the best course of action here, e.g. Labels can still be updated/reconciled, but we should make sure the .spec.template remains untouched.
The problematic bit is that .spec.template contains elements that are filled after the object is created e.g.:

spec:
  template:
    metadata:
      creationTimestamp: null
      labels:
        controller-uid: 7ec0fb0f-d0fa-4795-b051-233d8d40f19c

I'm looking for guidance on how to handle this situation properly 🙏

@csviri
Copy link
Collaborator

csviri commented Sep 19, 2022

We don't have a Job specific matcher yet, but this can be probably just fixed if you override the matcher.

What is your exact use case?

Asking because recreating a use case is not supported, what might be an use case some times.

@andreaTP
Copy link
Collaborator Author

I would like to simply skip the update if .spec.template differ, and, to make sure I never update that subpath (as it will result in an error all the time).

@csviri
Copy link
Collaborator

csviri commented Sep 19, 2022

Ahh ok, this is defintely should be possible to do even now, just overriding the match and the update function.

However might be easier if the desired would receive the actual resource too if present. cc @metacosm - will prepare a PR for this.

@csviri csviri linked a pull request Sep 19, 2022 that will close this issue
@csviri
Copy link
Collaborator

csviri commented Sep 19, 2022

@andreaTP I linked a PR, we can discuss that. Basically now this is possible just more hacky, with that it should be doable much more elegantly. Probably the best would be to add to the same branch an integration test for this use case.

@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 19, 2022
@github-actions
Copy link

github-actions bot commented Dec 4, 2022

This issue was closed because it has been stalled for 14 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 4, 2022
@csviri csviri removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 31, 2023
@csviri csviri reopened this Jan 31, 2023
@csviri csviri added this to the 4.3 milestone Jan 31, 2023
@csviri
Copy link
Collaborator

csviri commented Jan 31, 2023

I was updating the API for this use case, see related PR. But realized that getSecondaryResource always can be called. So you can just call that set the spec.template part from there if present. In this case it is not changed.
Not sure if we should still change the API of desired. Will create an example at least.

@csviri
Copy link
Collaborator

csviri commented Jan 31, 2023

See sample here:
#1746

I think this is good enough, no need to explicitly put the resource in to desrire(...) api. Unless you think otherwise @andreaTP or @metacosm

@csviri csviri linked a pull request Jan 31, 2023 that will close this issue
@csviri csviri self-assigned this Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment