> On July 20, 2016, 11:17 p.m., Alexandr Antonenko wrote: > > ambari-web/app/utils/config.js, line 264 > > <https://reviews.apache.org/r/49387/diff/3-4/?file=1445954#file1445954line264> > > > > 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 > > Keta Patel wrote: > The updated patch "AMBARI-17041-July21-updated.patch" conatins this > correction. I have kept the original statement with the name of the variable > updated to "attributes[0][FINAL]". > > Thank you! > > Alexandr Antonenko wrote: > You are using your newly created variable FINAL only one time in > attributes[0][FINAL][index]. Why not simply use > attributes[0]["FINAL"][index], passing needed string directly, without > creating variable FINAL. especially when you have file with tons of variables > and functions which names have FINAL in it. > > Keta Patel wrote: > Hello Alexandr, > I have updated the latest patch "AMBARI-17041-July22.patch" by removing > the "FINAL" variable. I have still kept the variable "PASSWORD" as it is used > in 2 functions (just once in both functions though). > > Thank you!
Good job - Alexandr ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49387/#review143024 ----------------------------------------------------------- On July 25, 2016, 8:47 p.m., Keta Patel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49387/ > ----------------------------------------------------------- > > (Updated July 25, 2016, 8:47 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 > aba45bf > > 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 patch "AMBARI-17041-trunk-July08.patch" the following is result of > ambari-web tests: > > 29017 tests complete (48 seconds) > 154 tests pending > > With the 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 patch "AMBARI-17041-trunk-July20.patch" the following is the result > of ambari-web tests: > 29292 tests complete (42 seconds) > 154 tests pending > > With the patch "AMBARI-17041-July21-updated.patch" the following is the > result of ambari-web tests: > 29343 tests complete (55 seconds) > 154 tests pending > > **With the patch "AMBARI-17041-July22.patch" the following is the result of > ambari-web tests: > 29371 tests complete (43 seconds) > 154 tests pending > > > File Attachments > ---------------- > > AMBARI-17041-July21-ES6.patch > > https://reviews.apache.org/media/uploaded/files/2016/07/21/866014d0-310d-4e05-8290-2f82de990824__AMBARI-17041-July21-ES6.patch > > > Thanks, > > Keta Patel > >
