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

Node pool name customisation #462

Merged
merged 7 commits into from
Nov 17, 2022
Merged

Node pool name customisation #462

merged 7 commits into from
Nov 17, 2022

Conversation

melzayet
Copy link
Contributor

@melzayet melzayet commented Nov 15, 2022

PR Summary

User node pool name should be customisable

PR Checklist

  • PR has a meaningful title
  • Summarized changes
  • This PR is ready to merge and is not Work in Progress
  • Link to a filed issue:
  • Screenshot of UI changes (if PR includes UI changes)

As described in issue #461, user node pool is made configurable

image

In case of only system node pools, the parameter is not set.

If no value provided by user, it defaults to npuser01, the original string that was used

@Gordonby
Copy link
Collaborator

Logic looks good.
I'll fire up a couple of tests then we can merge. Great PR 😀

@melzayet
Copy link
Contributor Author

@Gordonby one thing I am was not sure of is whether the text input field should be required (with the red asterik).

It's required unless only system node pool exists. It's currently always required. What's your thoughts?

@Gordonby
Copy link
Collaborator

Gordonby commented Nov 16, 2022

I actually think we can lean into the system pool a little more. It might make the required bit a little easier.

In the scenario of just a system pool being deployed;

  • We relabel the textbox to "Node Pool Name"
  • We allow the system pool to also be renamed
    image

Thoughts?

Copy link
Collaborator

@Gordonby Gordonby left a comment

Choose a reason for hiding this comment

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

Can we add an output of the pool name(s) ?
This way if a consuming bicep/pipeline uses AKSC then it'll be easy to grab the poolnames.

@melzayet
Copy link
Contributor Author

@Gordonby suggestions absolutely make sense. I have implemented, tested and committed those.

I just want to highlight the commenting out of code block startling line 50 in clusterTab.js. This was clearing vmSKU without any changes in compute type

@Gordonby Gordonby changed the title user node pool name can be configured Node pool name customisation Nov 17, 2022
Gordonby
Gordonby previously approved these changes Nov 17, 2022
@Gordonby Gordonby added the enhancement New feature or request label Nov 17, 2022
@Gordonby Gordonby merged commit 5749f09 into main Nov 17, 2022
@Gordonby Gordonby deleted the mz-np-naming branch November 17, 2022 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request helper-ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants