Skip to content
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

Vendor apply.DryRun #2854

Merged
merged 2 commits into from
Sep 27, 2024
Merged

Vendor apply.DryRun #2854

merged 2 commits into from
Sep 27, 2024

Conversation

manno
Copy link
Member

@manno manno commented Sep 16, 2024

Fleet's helmdeployer used the wranger/apply module to do a "DryRun" to find modified objects.
Apply is very configurable, however Fleet only used a specific set of options. The relevant code paths to build the "plan" have been copied into the "deployer" packager. Several options, parameters, interfaces and switches have been removed.

  • copy helmdeployer's pkgs from wrangler
  • summary, had to move to the API pkg, as API cannot import fleet
  • apply "patcher" was always replaced with an "append" function, patchers were never used
  • delete is never used since ErrReplaced is never returned
  • two structs implement "Apply" for the sugared interface, we only need
    one as "apply" is embedded in "desiredset"
  • "original" is never used in patches
  • rename applied package to desiredset
  • fix/exclude linter warnings
  • remove context from struct, pass in context directly
  • replace logrus debug messages and increase verbosity

Notes

  • this affects how detect drift finds deleted resources
  • this affects how modified resources are detected
  • the patch: field is populated by the "dryrun" code, but always overwritten by the "diff" code. It's only calculated to fill the plan.Update struct. That struct is similar to the plan.Objects' struct and Diffalready skips non modified resources. Data structures are different enough to make an optimization complicated, though. So removingplan.Update` is not part of this PR.
  modifiedStatus:
  - apiVersion: v1
    kind: ConfigMap
    name: test-simple-manifest-config
    namespace: simple
    patch: '{"data":{"test":"value"}}'

@manno manno force-pushed the vendor-apply branch 6 times, most recently from c084781 to dea5095 Compare September 23, 2024 09:52
@manno manno marked this pull request as ready for review September 23, 2024 09:56
@manno manno requested a review from a team as a code owner September 23, 2024 09:56
Fleet's helmdeployer used the wranger/apply module to do a "DryRun" to find modified objects.
Apply is very configurable, however Fleet only used a specific set of options. The relevant code paths to build the "plan" have been copied into the "deployer" packager. Several options, parameters, interfaces and switches have been removed.

* copy helmdeployer's pkgs from wrangler
* summary, had to move to the API pkg, as API cannot import fleet
* apply "patcher" was always replaced with an "append" function, patchers were never used
* delete is never used since ErrReplaced is never returned
* two structs implement "Apply" for the sugared interface, we only need
* one as "apply" is embedded in "desiredset"
* "original" is never used in patches
* rename applied package to desiredset
* fix/exclude linter warnings
* remove context from struct, pass in context directly
* replace logrus debug messages and increase verbosity
p-se
p-se previously approved these changes Sep 24, 2024
Copy link
Contributor

@p-se p-se left a comment

Choose a reason for hiding this comment

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

lgtm

@manno manno merged commit 5d934fa into main Sep 27, 2024
12 checks passed
@manno manno deleted the vendor-apply branch September 27, 2024 14:18
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.

2 participants