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

Fixes #17804: Trying to add a group property with change request plugin enabled lead to blank property #3096

Conversation

fanf
Copy link
Member

@fanf fanf commented Jun 22, 2020

@fanf fanf requested a review from VinceMacBuche June 22, 2020 20:16
@fanf
Copy link
Member Author

fanf commented Jun 22, 2020

Must be validated along with Normation/rudder-plugins#317

@fanf
Copy link
Member Author

fanf commented Jun 22, 2020

PR updated with a new commit

).toBox
val name = (p \\ "name").text.trim
if(name.isBlank) {
Failure(s"Found unexpected xml under <properties> tag (name is blank): ${p}")
Copy link
Member Author

Choose a reason for hiding this comment

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

Forbid to same load a property with an empty name, it brokes things all around

@@ -405,10 +405,13 @@ final case class RestDataSerializerImpl (
val serverList :JValue = diff.modNodeList.map(displaySimpleDiff(_)(convertNodeList)).getOrElse(initialState.serverList.map(v => (v.value)).toList)
val dynamic :JValue = diff.modIsDynamic.map(displaySimpleDiff(_)).getOrElse(initialState.isDynamic)
val enabled :JValue = diff.modIsActivated.map(displaySimpleDiff(_)).getOrElse(initialState.isEnabled)
val properties: JValue = diff.modProperties.map(displaySimpleDiff(_)).getOrElse(initialState.properties.toApiJson())

Copy link
Member Author

Choose a reason for hiding this comment

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

missings bits to display properties diff

@@ -154,9 +154,9 @@ class NodeGroupForm(

val pendingChangeRequestXml =
<div id="pendingChangeRequestNotification">
<div>
<div class="row">
<i class="fa fa-exclamation-triangle warnicon" aria-hidden="true"></i>
Copy link
Member Author

Choose a reason for hiding this comment

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

the warning about existing change request was broken because of group UI update

@fanf
Copy link
Member Author

fanf commented Jun 22, 2020

PR updated with a new commit

@fanf
Copy link
Member Author

fanf commented Jun 22, 2020

something is not working with property update

@fanf
Copy link
Member Author

fanf commented Jun 22, 2020

ok, no, it's just that if you don't deploy change but just validate it, it is not deployed. Obviously.

@@ -178,6 +176,23 @@ nodePropertiesApp.controller('nodePropertiesCtrl', function ($scope, $http, DTOp
}
}
};

$scope.processResponse = function(response, successMsg) {
// check if it created a change request
Copy link
Member Author

Choose a reason for hiding this comment

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

we need to redirect to change request screen if it's a change request.

var changeId = undefined;
try {
changeId = response.data.data[$scope.objectName+'s'][0].changeRequestId;
} catch {}
Copy link
Member

@ElaadF ElaadF Jun 23, 2020

Choose a reason for hiding this comment

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

Should we not log something here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. In case we don't have a change request, we are not sure about the format of answer

@fanf
Copy link
Member Author

fanf commented Jun 23, 2020

PR updated with a new commit

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/3096
-- Your faithful QA
Kant merge: "Live your life as though your every act were to become a universal law."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/26751/console)

@fanf
Copy link
Member Author

fanf commented Jun 25, 2020

OK, squash merging this PR

@fanf fanf force-pushed the bug_17804/trying_to_add_a_group_property_with_change_request_plugin_enabled_lead_to_blank_property branch from 94c4d29 to 0650059 Compare June 25, 2020 09:09
@fanf fanf merged commit 0650059 into Normation:branches/rudder/6.1 Jun 25, 2020
@fanf fanf deleted the bug_17804/trying_to_add_a_group_property_with_change_request_plugin_enabled_lead_to_blank_property branch March 15, 2024 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants