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

Remove watchResource and associated code. #3897

Merged
merged 3 commits into from
Dec 6, 2021

Conversation

absoludity
Copy link
Contributor

Description of the change

Following on from the other #3779 PRs, this PR removes more now-unused code (after switching to the new resources plugin endpoint) and simplifies the keyForResourceRef helper now that it is used solely with our protobuf-generated resourceref.

Benefits

Possible drawbacks

Applicable issues

Additional information

Signed-off-by: Michael Nelson <minelson@vmware.com>
Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Great! Happy to be removing plenty of complex code in the UI (and tests....). Thanks for the clean-up!

return refs
.map(r => resources[keyForResourceRef(r.apiVersion, r.kind, r.namespace, r.name)])
.filter(r => r !== undefined);
return refs.map(r => resources[keyForResourceRef(r)]).filter(r => r !== undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know about our preference of using lodash vs plain js, but I guess you can also do sth like _.omitBy. Anyway, it is clear enough and easy to understand, so that's totally fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was the existing code that needed to handle undefined resources in the state. I'm not sure how the state could previously get into this situation, but I'm pretty confident that it cannot do so now anyway (as we only add resources to the state in the first place if they are sent by the resources plugin), but lots of test data currently depends on it. I may remove it in another PR and update the tests.

Comment on lines 124 to 125
// We book keep on subscriptions, keyed by the installed package ref,
// so that we can
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this comment is stripped out? I think I don't get the sentence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry - probably got distracted half way through writing it :P. Fixed.

Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity absoludity merged commit e9cf700 into master Dec 6, 2021
@absoludity absoludity deleted the 3779-dashboard-deletion-3 branch December 6, 2021 00:29
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.

None yet

2 participants