-
Notifications
You must be signed in to change notification settings - Fork 110
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
feat: new config for excluding fields on history-less resources #760
feat: new config for excluding fields on history-less resources #760
Conversation
Hello @praveenrewar! I would like to have a brief review on this draft. I would like to ensure, that I am on the right track here. Unit tests are green, e2e tests are running right at the moment. Of course, I will add unit tests for this specific feature, too. Additionally, I would like to refactor it a bit more, but first let's see if the overall approach is the right one ;-) |
Hey @theurichde! Thank you so much for working on this ❤️ |
Hi @theurichde, the overall approach looks good to me, as you have mentioned we could do some refactoring, but I think we are in the right direction 🚀 We are completely ignoring the status field from diff, but we should probably ensure that |
87cf0f3
to
76c9820
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that the custom_wait_rules_test and order_test are creating a hindrance in this PR and they are not even related to the changes being made. Do you want to pair to quickly wrap this or would you be comfortable if I made a separate (quick) PR to refactor them so they are not effected by the changes being made in this PR (You have spent some time working on them so it's totally fine if you would want to continue and finish it here itself :) )?
I just created a PR to refactor those tests, let me know if you would want me to merge it and rebase this PR on top of it or feel free to copy those changes. |
Okidoki, go for it 😍 |
Done 🚀 |
f33db2f
to
f910030
Compare
…rces Signed-off-by: Tim Heurich <[email protected]>
f910030
to
eb2e9d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for working on this @theurichde 🙇🏻
What this PR does / why we need it:
This PR introduces a new diff field exclusion configuration for resources without a valid history
Which issue(s) this PR fixes:
Fixes #746
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: