-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49387/#review143024
-----------------------------------------------------------




ambari-web/app/utils/config.js (line 248)
<https://reviews.apache.org/r/49387/#comment208704>

    FYI: this can be also simplified 
    
    var attributes = [];
    ['FINAL', 'PASSWORD', 'USER', 'GROUP', 'TEXT', 'ADDITIONAL_USER_PROPERTY', 
'NOT_MANAGED_HDFS_PATH', 'VALUE_FROM_PROPERTY_FILE'].forEach(function 
(propertyName) {
       attributes.push(
       {
        [propertyName] : Em.get(configJSON, 
'properties_attributes.'+propertyName) || {}})
    })



ambari-web/app/utils/config.js (line 264)
<https://reviews.apache.org/r/49387/#comment208700>

    I do not see FINAL populated anywhere. I think this code will trigger fatal 
error. Have you tested this part manually ? 
    
    Also, there is no need to check attribute[0], you just created it, and 
anyway there will be one and more values in that array
    
    In code : attributes[0][FINAL][index] && attributes[0][FINAL][index] === 
"true"
    
    You can skip cheking attributes[0][FINAL][index], just check 
attributes[0][FINAL][index] === "true".
    
    If property "attributes[0][FINAL][index]" does not exist 
attributes[0][FINAL][index] === "true" will not trigger any error


- Alexandr Antonenko


On July 20, 2016, 9:05 p.m., Keta Patel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49387/
> -----------------------------------------------------------
> 
> (Updated July 20, 2016, 9:05 p.m.)
> 
> 
> Review request for Ambari, Alexandr Antonenko, Di Li, Dmitro Lisnichenko, and 
> Jonathan Hurley.
> 
> 
> Bugs: AMBARI-17041
>     https://issues.apache.org/jira/browse/AMBARI-17041
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Currently, services can define properties in the XML configuration files that 
> is flagged as type password:
>   <property>
>         <name>my.special.password</name>
>         <value></value>
>         <property-type>PASSWORD</property-type>
>         <description>Password to be masked</description>
>     </property> 
> and it will be masked properly in the UI as well as blueprint.
> 
> Custom property should also support this option so that password can be added 
> as custom property and treat accordingly.
> 
> 
> ==========================================
> Proposed Design for the fix:
> ==========================================
> At present only the key-value information of the service properties is stored 
> in the DB ("clusterconfig" table in the "config_data" column). 
> The "config_attributes" column stores only certain attributes like "final" 
> indicating the list of properties set with the Final flag = true. 
> The information about the property-type (i.e PASSWORD, USER, GROUP, 
> ADDITIONAL_USER_PROPERTY, VALUE_FROM_PROPERTY_FILE, NOT_MANAGED_HDFS_PATH, 
> etc) is extracted from the corresponding service's property file (e.g. 
> hive-site.xml, core-site.xml, webhcat-env.xml, etc). These files contain 
> information of the existing properties only. Custom Properties added by 
> ambari user have no provision to store their additional attributes. 
> 
> Since, for this Jira we are concerned with only <property-type> attribute for 
> Custom Properties, we could add an additional field called "Property Type" in 
> the "Add Property" pop-up which shows up on clicking "Add Property ..." in 
> the Custom property section for a service. For now, only 2 options are shown 
> in the drop-down list: NONE and PASSWORD .
> A few sample test properties are created using the new "Add Property" pop-up 
> as can be seen in the following attachments. 
> Attachments: 
> "add_property_pop_up.tiff"
> "custom_property_password_type.tiff"
> "custom_property_regular_type.tiff"
> "custom_properties_after_save.tiff"
> 
> The <property-type> information for these Custom properties is stored in the 
> DB in "clusterconfig" table, "config_attributes" column.
> The schema for "clusterconfig" table can be seen in the attachment:
> "schema_of_clusterconfig_table.tiff"
> 
> The content of the "config_attributes" column with the <property-type> 
> information from the new Custom properties can be seen in the attachment:
> "cluster_config_with_password_type_in_config_attributes_column.tiff"
> 
> 
> Note: The fix so far is performed only for new Custom properties. The 
> <property-type> information for existing properties is extracted from the 
> corresponding property xml files for the service.
> 
> 
> Diffs
> -----
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
>  066acab 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/ConfigurationResponse.java
>  eef3474 
>   
> ambari-server/src/main/java/org/apache/ambari/server/utils/SecretReference.java
>  84f3109 
>   ambari-web/app/messages.js 24c9cfa 
>   ambari-web/app/mixins/common/configs/configs_saver.js 43a77ce 
>   ambari-web/app/models/configs/objects/service_config_property.js 844806f 
>   ambari-web/app/templates/common/configs/addPropertyWindow.hbs 659435c 
>   ambari-web/app/utils/config.js 9bb977a 
>   ambari-web/app/views/common/configs/service_configs_by_category_view.js 
> 964e193 
>   ambari-web/test/mixins/common/configs/configs_saver_test.js 4baf303 
> 
> Diff: https://reviews.apache.org/r/49387/diff/
> 
> 
> Testing
> -------
> 
> No new test cases are written in the patch apart from the fix required in 
> "configs_saver_test.js" to avoid test failure of existing tests.
> If the proposed solution is acceptable, I will create a new patch with the 
> necessary new tests.
> 
> The existing ambari-web tests after applying the patch:
> 
>   28977 tests complete (37 seconds)
>   154 tests pending
>   
>   
> With the new latest patch "AMBARI-17041-trunk-July08.patch" the following is 
> result of ambari-web tests:
> 
>   29017 tests complete (48 seconds)
>   154 tests pending
>   
> With the new patch "AMBARI-17041-trunk-July13.patch" the following is the 
> result of ambari-web tests:
> 
>   29021 tests complete (26 seconds)
>   154 tests pending
>   
> **With the new patch "AMBARI-17041-trunk-July20.patch" the following is the 
> result of ambari-web tests:
>   29292 tests complete (42 seconds)
>   154 tests pending
> 
> 
> Thanks,
> 
> Keta Patel
> 
>

Reply via email to