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: [WD-19015] CMS Server Config for maas.machine #1097

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

Kxiru
Copy link
Contributor

@Kxiru Kxiru commented Feb 10, 2025

Done

  • Introduced MaasMachineSettingFormInput.tsx

Fixes [list issues/bugs if needed]

  • Maas Machine Setting.
  • Storage.*volume settings.
  • .*_address settings.

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • [PENDING]

Screenshots

image
image

@webteam-app
Copy link

@Kxiru Kxiru force-pushed the cms-fields-maas branch 2 times, most recently from 16317a6 to 825dc74 Compare February 14, 2025 10:28
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Good progress on this one! Some comments below.

@Kxiru Kxiru requested a review from edlerd February 17, 2025 15:08
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Thanks for the iteration! Some hints how to get this finished and more generalized below.

@Kxiru Kxiru force-pushed the cms-fields-maas branch 2 times, most recently from 19d9de3 to 281d9d0 Compare February 18, 2025 01:50
@Kxiru
Copy link
Contributor Author

Kxiru commented Feb 18, 2025

@edlerd , Line 64 has a lint error for the use of Stirng(), however this cannot just be replaced to JSON.stringify because it then means that all the non-clustered values that take string input end up having their speech marks included in the Value field when in non-edit mode, and their default setting becomes ' "" ' instead of '-'. I would suggest trying this locally and testing with maas.api.url.
Do you have any ideas for how to get around this lint error?

@Kxiru Kxiru requested a review from edlerd February 18, 2025 01:59
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Good progress, the API layer seems all right. Some suggestions to fix the layout in read mode and to simplify a bit more.

@Kxiru Kxiru force-pushed the cms-fields-maas branch 3 times, most recently from 9dee7a6 to 713dd0f Compare February 19, 2025 16:32
@Kxiru Kxiru requested a review from edlerd February 19, 2025 16:32
@Kxiru Kxiru marked this pull request as ready for review February 19, 2025 17:31
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Code and QA work nicely, thanks for the changes. Some suggestions to simplify below.

Ideally we make the empty read only state, when no value is set also display "-", to stay consistent with other parts of the app.

@Kxiru Kxiru requested a review from edlerd February 19, 2025 18:13
edlerd
edlerd previously approved these changes Feb 19, 2025
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

LGTM, good work 👍

@Kxiru Kxiru requested a review from mas-who February 20, 2025 14:06
Copy link
Contributor

@mas-who mas-who left a comment

Choose a reason for hiding this comment

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

LGTM on disabling the cluster settings 👍

@Kxiru Kxiru merged commit ea54c7e into canonical:main Feb 20, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants