MAAS

Merge ~newell-jensen/maas:no-arch-and-mac-address-validation-with-ipmi-power-parameters into maas:master

Git
lp:~newell-jensen/maas
no-arch-and-mac-address-validation-with-ipmi-power-parameters
Merge into master

Proposed by Newell Jensen on 2018-06-07

Status:

Rejected

Rejected by:

Newell Jensen on 2018-07-02

Proposed branch:

~newell-jensen/maas:no-arch-and-mac-address-validation-with-ipmi-power-parameters

Merge into:

maas:master

Diff against target:

490 lines (+100/-135)

11 files modified

src/maasserver/api/machines.py (+6/-0)
src/maasserver/api/tests/test_machines.py (+27/-0)
src/maasserver/forms/__init__.py (+28/-13)
src/maasserver/forms/tests/test_machine.py (+1/-1)
src/maasserver/forms/tests/test_machinewithmacaddresses.py (+19/-1)
src/maasserver/models/node.py (+1/-13)
src/maasserver/models/tests/test_node.py (+0/-23)
src/maasserver/static/js/angular/controllers/add_hardware.js (+11/-28)
src/maasserver/static/js/angular/controllers/tests/test_add_hardware.js (+6/-24)
src/maasserver/static/partials/nodes-list.html (+1/-1)
src/maasserver/websockets/handlers/tests/test_machine.py (+0/-31)

Low

Fix Released

Link a bug report

Reviewer	Date Requested	Status
MAAS Lander		Approve on 2018-06-17
Lee Trager (community)	2018-06-07	Needs Fixing on 2018-06-15
Review via email: mp+347649@code.staging.launchpad.net

Commit message

LP: #1707216 - No validation for architecture or mac addresses is required for IPMI power types.

Revision history for this message

MAAS Lander (maas-lander) wrote on 2018-06-07:

UNIT TESTS
-b no-arch-and-mac-address-validation-with-ipmi-power-parameters lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 7b0fdba3debce6fb4818b6cc6b3300c454d5de48

review: Approve

Revision history for this message

Lee Trager (ltrager) wrote on 2018-06-13:

I'm not sure clearing the errors after they occur is the best path. If I add form.save() to your unit test an exception is raised about the architecture missing. I think MAAS should be smart enough to know when to avoid the errors.

On Django forms whether or not a field is required is controlled by the optional field argument required. On the mac_addresses field this is not set so Django is assuming it always is, you can turn it off by modifying WithMACAddressesMixin.set_up_mac_addresses_field with

self.fields['mac_addresses'] = MultipleMACAddressField(len(macs), required=(self.data.get('power_type') != 'ipmi'))

If you look at MachineForm.set_up_architecture_field you'll see required is already False. The error is coming from Node.clean_architecture. That is why clearing the error won't help, the model save will still raise the error. The form should be validating the architecture already so I think its safe to remove Node.clean_architecture and Device.clean_architecture and rely on the form by making required=True unless the power type is ipmi.

review: Needs Fixing

Revision history for this message

MAAS Lander (maas-lander) wrote on 2018-06-14:

UNIT TESTS
-b no-arch-and-mac-address-validation-with-ipmi-power-parameters lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: a12d6d70413c1cd5fe1695c6000f522af1799ca1

review: Approve

Revision history for this message

MAAS Lander (maas-lander) wrote on 2018-06-14:

UNIT TESTS
-b no-arch-and-mac-address-validation-with-ipmi-power-parameters lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 18315b3cef069a5695ab5ee5f21ddbf1a5473b14

review: Approve

Revision history for this message

MAAS Lander (maas-lander) wrote on 2018-06-14:

UNIT TESTS
-b no-arch-and-mac-address-validation-with-ipmi-power-parameters lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/3164/console
COMMIT: 97fb812e49d1a49dbfd6211157540dfe5fe7ad5f

review: Needs Fixing

Revision history for this message

MAAS Lander (maas-lander) wrote on 2018-06-14:

UNIT TESTS
-b no-arch-and-mac-address-validation-with-ipmi-power-parameters lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: f2e44fad675b05b437679f0993817f3dcf15c991

review: Approve

Revision history for this message

Lee Trager (ltrager) wrote on 2018-06-15:

If MAC addresses are added and an architecture isn't specified in the UI MAC addresses shouldn't be added for the machine. You stopped adding them only over the API, this should be done in the form. A few other comments below but I think this is close.

review: Needs Fixing

Revision history for this message

Newell Jensen (newell-jensen) wrote on 2018-06-17:

I updated the branch with the review fixes. Thanks for the review Lee.

Revision history for this message

MAAS Lander (maas-lander) wrote on 2018-06-17:

UNIT TESTS
-b no-arch-and-mac-address-validation-with-ipmi-power-parameters lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 94cf8d46212d7476f9720cc44c3334c803630c20

review: Approve

Revision history for this message

Newell Jensen (newell-jensen) wrote on 2018-07-02:

Rejecting this branch as my changes were used in https://code.launchpad.net/~ltrager/maas/+git/maas/+merge/348256