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

Switch to use <EuiSuperselect /> component to list docker registery in router deployment #396

Merged

Conversation

vinoth-gojek
Copy link
Contributor

@vinoth-gojek vinoth-gojek commented Nov 11, 2024

At the time of writing, clicking Docker Image Registry list in Turing UI leads to an error page. This happens because of "Maximum update depth exceeded" which happens usually as infinte re-rendering is blocked. While toggling the popover, the state re-rendering is causing infinte reload. I couldn't find any extra value addition in using "EuiPopover" over the simple EuiSuperSelect.

The MR fixes the issue by choosing switching from "EuiPopover" to use "EuiSuperSelect" component

Test plan - https://drive.google.com/file/d/1CLl-RoVwO8dhiDmv8YUMJRNT3IJdMiK3/view?usp=sharing (limited access)

@vinoth-gojek vinoth-gojek force-pushed the DAT-2033_fix_unexpected_error_while_clicking_artifactory branch from bec016a to e20b466 Compare November 11, 2024 09:28
@vinoth-gojek vinoth-gojek force-pushed the DAT-2033_fix_unexpected_error_while_clicking_artifactory branch from e20b466 to add1b11 Compare November 11, 2024 09:30
@vinoth-gojek vinoth-gojek changed the title switch to use euisuperselect to list docker registery switch to use euisuperselect to list docker registery in Router Deployment Nov 11, 2024
@vinoth-gojek vinoth-gojek marked this pull request as ready for review November 11, 2024 09:56
@vinoth-gojek vinoth-gojek changed the title switch to use euisuperselect to list docker registery in Router Deployment Switch to use euisuperselect to list docker registery in router deployment Nov 11, 2024
@vinoth-gojek vinoth-gojek changed the title Switch to use euisuperselect to list docker registery in router deployment Switch to use <EuiSuperselect /> component to list docker registery in router deployment Nov 11, 2024
@deadlycoconuts
Copy link
Contributor

Thanks for this PR! Hmm do you have any idea what's causing the infinite re-rendering? The popover's supposed to have an effect that's similar to what's shown in the video below:

Screen.Recording.2024-11-14.at.10.38.42.mov

You can take a look at the code snippet here corresponding to that part above (which looks super similar to what you're working on) and maybe you might figure out what's causing the popover to break 🤔

@vinoth-gojek
Copy link
Contributor Author

**deadlycoconuts ** commented Nov 14, 2024

@deadlycoconuts yeah valid point, I was unsuccessful cornering the setstate which is causing the exception. I suspect, the dropdown updates registry, on change of registry, it re-renders the parent component, but as mentioned I was not able to corner it. So I moved away to use "EuiSuperselect" which helped

Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
    at checkForNestedUpdates (http://localhost:3001/turing/static/js/vendors-node_modules_react-dom_index_js.chunk.js:23602:15)

@vinoth-gojek
Copy link
Contributor Author

vinoth-gojek commented Nov 18, 2024

Attaching more manual testing details for adding new registry

  1. New registry could be configured from privateDockerRegistries config
  2. Adding a new registry will look like - image
  3. Successful deployment of router with new registry with publicly available docker image - link

One caveat/catch is that config only allows urls to be passed and no other option to pass user friendly name. This is something we could consider refactoring in future

Copy link
Contributor

@deadlycoconuts deadlycoconuts left a comment

Choose a reason for hiding this comment

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

Thanks a lot for finding an alternative component from the EUI library to replace the EuiPopover/EuiContextMenu combination that somehow isn't working. I believe there's some issue in the parent component using the DockerRegistryPopover that's probably triggering unnecessary re-renders but we'll probably have to come back to this another time :/

@vinoth-gojek
Copy link
Contributor Author

hi @deadlycoconuts thanks for sharing the comments. I have addressed your comments with the latest commit. I have kept those open for you to take a look again. Thanks and let me know your thoughts in case there are any more to address!

@deadlycoconuts
Copy link
Contributor

Thanks a lot for addressing them! I think we're good to merge this PR! Once this is done we can cut a patch release like this one here https://github.com/caraml-dev/turing/releases/tag/v1.20.2!

@vinoth-gojek vinoth-gojek merged commit be214e9 into main Nov 26, 2024
13 checks passed
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