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

feat: add the slots property to the props #8498

Merged
merged 3 commits into from
Dec 5, 2023

Conversation

thiagodallacqua-hpe
Copy link
Contributor

@thiagodallacqua-hpe thiagodallacqua-hpe commented Nov 29, 2023

Description

Add the slots property as a fallback option for rendering the node/agents slots and added a check for empty array in the context of the current RP filtering.

Test Plan

  • Go to the Clusters page
  • select a Resource Pool
  • if there's no experiment running and/or no agent/node is being commissioned, there should be no Topology section being rendered.

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

@cla-bot cla-bot bot added the cla-signed label Nov 29, 2023
Copy link

netlify bot commented Nov 29, 2023

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 7f9c64b
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/656f69c45acb760008b4b96d
😎 Deploy Preview https://deploy-preview-8498--determined-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@hkang1 hkang1 left a comment

Choose a reason for hiding this comment

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

mostly looks good, but couple of questions regarding poolname being undefined for ResourcepoolDetail component

nodes={agents.filter(({ resourcePools }) => resourcePools.includes(poolname))}
/>
)}
{!!topologyAgentPool.length && poolname && <Topology nodes={topologyAgentPool} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

question: this looks better. I'm not too familiar here, but it's kind of odd that the ResourcepoolDetail.tsx doesn't make poolname required and is optional. Is there a case for the card to handle undefined value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just following the type 😅 , I also think that it doesn't make much sense having it as undefined...

string | undefined

webui/react/src/types.ts Outdated Show resolved Hide resolved
webui/react/src/pages/ResourcePool/Topology.tsx Outdated Show resolved Hide resolved
@hkang1 hkang1 assigned thiagodallacqua-hpe and unassigned hkang1 Dec 5, 2023
Copy link
Contributor

@hkang1 hkang1 left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for the updates

@thiagodallacqua-hpe thiagodallacqua-hpe merged commit a6ae0c7 into main Dec 5, 2023
71 of 82 checks passed
@thiagodallacqua-hpe thiagodallacqua-hpe deleted the thiago/topology-type-fix branch December 5, 2023 18:32
@azhou-determined azhou-determined added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Dec 5, 2023
dai-release bot pushed a commit that referenced this pull request Dec 5, 2023
azhou-determined pushed a commit that referenced this pull request Dec 7, 2023
@dannysauer dannysauer added this to the 0.26.7 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed to-cherry-pick Pull requests that need to be cherry-picked into the current release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants