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

Refactor IgnoreChanges #1639

Merged
merged 8 commits into from
Jan 25, 2024
Merged

Refactor IgnoreChanges #1639

merged 8 commits into from
Jan 25, 2024

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Jan 23, 2024

The intent here is to refactor the code without any observable changes to make possible further extensions such as #1614 .

Specifically, before this PR:

type Provider interface {
        ...
	Diff(t string, s InstanceState, c ResourceConfig) (InstanceDiff, error)
}

type InstanceDiff interface {
        ...
	IgnoreChanges(ignored map[string]bool)
}

That is the interface is to compute a diff first, and then side-effect to ignore changes.

After this PR we have this instead:

type Provider interface {
        ...
	Diff(t string, s InstanceState, c ResourceConfig, opts DiffOptions) (InstanceDiff, error)
}
type IgnoreChanges = func() map[string]struct{}
type DiffOptions struct {
  IgnoreChanges IgnoreChanges
}

So the algorithm to ignore changes is specified upfront when constructing the diff. This seems to be useful to un-constrain the implementation of shim.Provider a little bit, specifically in #1614 that is trying to change the internal representation of InstanceDiff implementation in such a way that it is inconvenient to process IgnoreChanges after the InstanceDiff is constructed already.

@t0yv0
Copy link
Member Author

t0yv0 commented Jan 23, 2024

Opening for early feedback. We don't have to merge yet I guess until I have an end to end demonstration that 1614 is viable on top of this (I will rebase). But this seems to me like it could help.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

I left a couple of comments, but I'm over all in favor of a refactor like this.

pkg/tfshim/shim.go Outdated Show resolved Hide resolved
pkg/tfshim/shim.go Outdated Show resolved Hide resolved
pkg/tfbridge/diff.go Outdated Show resolved Hide resolved
t0yv0 added 7 commits January 25, 2024 15:20
Taking a deferred breaking change here:

- remove shim.ProviderWithContext
- make shim.Provider require Context in all runtime methods
- shim.Provider NewDestroyDiff now accepts a token to specialize it for a type
@t0yv0 t0yv0 force-pushed the t0yv0/refactor-ignore-changes branch from d223e57 to 59c21dc Compare January 25, 2024 20:20
@t0yv0 t0yv0 marked this pull request as ready for review January 25, 2024 20:26
@t0yv0 t0yv0 requested a review from iwahbe January 25, 2024 20:26
@t0yv0
Copy link
Member Author

t0yv0 commented Jan 25, 2024

@iwahbe PTAL, I simplified this a bit and removed old way of doing things instead of deprecating it.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

LGTM

@t0yv0 t0yv0 enabled auto-merge (squash) January 25, 2024 20:41
@t0yv0 t0yv0 merged commit b23493a into master Jan 25, 2024
7 checks passed
@t0yv0 t0yv0 deleted the t0yv0/refactor-ignore-changes branch January 25, 2024 20:42
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