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

Create settings deploy component #133

Merged
merged 1 commit into from
Aug 23, 2019
Merged

Create settings deploy component #133

merged 1 commit into from
Aug 23, 2019

Conversation

steverydz
Copy link
Contributor

@steverydz steverydz commented Aug 21, 2019

Done

Created the component for the configuration settings deploy form.

Drive by:

  • Moved the action dispatch for general config in the commissioning form from the CommissioningForm component to Commissioning.
  • Fixed a bug with updating the second value in the commissioning form

QA

  • Go to :5240/MAAS/#/images and find the Other images section near the bottom of the page, select all available images and save
  • Go to :8000/MAAS/settings/configuration/deploy
  • Change the value of Default operating system used for deployment and see that the options for Default OS release used for deployment are updated
  • Change values in both of the fields and save
  • Refresh and see that those values persist
  • Go to :5240/MAAS/settings/general and see that the form fields for the Deploy form have the same values as MAAS UI
  • Change the values here and save them
  • Go back to MAAS UI, refresh and see that the values are the same as what you changed them to in MAAS
  • Go to the commissioning form and check that works as expected

Copy link
Contributor

@Caleb-Ellis Caleb-Ellis left a comment

Choose a reason for hiding this comment

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

This is almost there. Just to repeat what was discussed on IRC, there is an issue where the formik values for default release aren't being updated when you switch default OS. This means you can send this through the websocket, for example:

{
  default_osystem: "centos"
  default_distro_series: "bionic"
}

which is causing an error.

The formik form values need to be explicitly updated when switching OS because I think the default behaviour is for the values to only update when they've been "touched".

ui/src/app/settings/selectors/config.js Outdated Show resolved Hide resolved
options={defaultOSystemOptions}
fieldKey="default_osystem"
formikProps={formikProps}
onChange={e => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@squidsoup @huwshimi I'm having an issue here. When changing the value for the top field, the formik value for the second field isn't updated.

I'm trying to do this manually using the built in methods that formik provides but distroSeriesOptions doesn't update in time for this to work. I've tried calling useSelector within the onChange handler but I can't actually do that because of how hooks work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I can't reproduce this. When I change the OS the series field does update for me (switches between centos 6/bionic).

Copy link
Contributor

Choose a reason for hiding this comment

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

@huwshimi the second field visibly updates, but the value within formikProps.values itself doesn't. You have to manually click the field for it to update.

Copy link
Contributor

Choose a reason for hiding this comment

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

@steverydz hey steve, did you resolve this issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@squidsoup yeah, I made a selector that returns all options formatted in a way that you can easily access what you want based on the key

Copy link
Contributor

@Caleb-Ellis Caleb-Ellis left a comment

Choose a reason for hiding this comment

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

Works great! My only suggestion now would be to update the general state once saving is complete, i.e. run actions.general.fetchOsInfo(). Otherwise general.osInfo remains in its old state until you navigate back to /deploy. This isn't really a huge issue since anywhere that needs general.osInfo will be probably fetch it on load, but Ithink it's nice to have redux state match reality where possible.
Anyway you can probably do it using an effect in Deploy.js that watches for config.saved.

@Caleb-Ellis
Copy link
Contributor

Having thought about this a bit more I think ignore what I said above. At the moment state is only updated on UPDATE_CONFIG_SYNC and we should keep that consistent. I think the point is still valid, that state should be as up to date as possible, but the solution doesn't need to belong in this PR.

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