Skip to content

Conversation

@milindk8
Copy link
Contributor

Description

Include cluster terminal FE-only fixes & enhancement

  1. Cluster terminal: Auto-select a node on which the terminal can be launched
  2. Show node-groups - Add node groups column on node list Ability to search nodes by node groups
    - Terminal > Node dropdown
    - Categorize nodes by node groups
    - Categorize ungrouped nodes in ‘Independent nodes’ category
    - If no node group exists, show all nodes without any categorization
  3. For custom images select sh as the default
  4. Ability to provide custom shell
  5. Messaging if the pod has been deleted
  6. When switching shell > Show message eg. Switching shell to bash
  7. If the selected shell is not supported, show the error message “{shellName} is not supported in the selected image”
  8. Connection timed out handling

Fixes #(AB1893)

Screenshot 2023-02-15 at 10 57 54 AM

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • By selecting the auto node option to select a node which connects automatically
  • With the BE help testing error cases for node connection

Checklist:

  • The title of the PR states what changed and the related issues number (used for the release note).
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

}
})

return [{label: '',options: [AUTO_SELECT]},...groupList]
Copy link
Contributor

Choose a reason for hiding this comment

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

Format this file

if (searchedTextMap.size !== matchedLabelCount) {
continue
}
} else if (selectedSearchTextType === 'nodeGroup') {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should create some constant for nodeGroup as it is being used on multiple places

terminalAccessId,
reconnectStart,
}: {
terminalAccessId: number
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this to type

setLoading(false)
const events = response.result?.events.items
const events = response.result
setEvents(events)
Copy link
Contributor

@vivek-devtron vivek-devtron Feb 23, 2023

Choose a reason for hiding this comment

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

we can directly set the data into setEvents no need to create the variable. also please accommodate events?.eventsResponse?.events.items it here only and and not at the time of passing the props on line no 45

})
}
toggleOptionChange()

Copy link
Contributor

@vivek-devtron vivek-devtron Feb 23, 2023

Choose a reason for hiding this comment

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

This is supposed to be useEffect's clean up function. right? Please move it out from try catch block

Copy link
Contributor Author

@milindk8 milindk8 Mar 2, 2023

Choose a reason for hiding this comment

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

no, it's not. It's just setting the state

value={selectedNodeName}
onChange={onChangeNodes}
styles={clusterSelectStyle}
styles={{
Copy link
Contributor

Choose a reason for hiding this comment

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

move this out to const

}

const renderErrorMessageStrip = (errorMessage) => {
if (errorMessage.message === 'timedOut') {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should move this to const

{TERMINAL_TEXT.CASE_OF_ERROR}
</div>
)
} else if (errorMessage.message === 'Terminated') {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

let sessionId = response.result.userTerminalSessionId
let status = response.result.status
if (!sessionId && count) {
if(status === 'Running' && !response.result?.isValidShell){
Copy link
Contributor

Choose a reason for hiding this comment

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

we should move statuses to const

if(status === 'Running' && !response.result?.isValidShell){
preFetchData(status,'failed')
setErrorMessage({message: response.result?.errorReason, reason: ''})
} else if (status === 'Terminated'){
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@milindk8 milindk8 requested a review from vivek-devtron March 2, 2023 09:07
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@milindk8 milindk8 linked an issue Mar 6, 2023 that may be closed by this pull request
2 tasks
@milindk8 milindk8 merged commit a6aef95 into main Mar 6, 2023
@milindk8 milindk8 deleted the cluster-terminal-enhancement branch March 6, 2023 14:50
@github-actions
Copy link

github-actions bot commented Mar 6, 2023

Coverage report

St.
Category Percentage Covered / Total
🔴 Statements 5.14% 1542/30015
🔴 Branches 2.93% 674/22970
🔴 Functions 2.95% 266/9002
🔴 Lines 5.22% 1495/28638

Test suite run success

67 tests passing in 12 suites.

Report generated by 🧪jest coverage report action from 93f21db

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.

Feature: Cluster terminal Iteration 2

5 participants