> On April 29, 2016, 11:18 p.m., Jaimin Jetly wrote:
> > ambari-web/app/mappers/stack_mapper.js, line 140
> > <https://reviews.apache.org/r/46460/diff/2/?file=1367769#file1367769line140>
> >
> >     A minor thing. This line can be changed to
> >     stack.use_redhat_satellite = 
> > !item.operating_systems[0].OperatingSystems.ambari_managed_repositories;
> >     
> >     Logiclly it's the same but this way it's just more readable.
> 
> Zhe (Joe) Wang wrote:
>     The reason I wrote it that is:
>     if item.operating_systems[0].OperatingSystems.ambari_managed_repositories 
> is undefined, we want stack.use_redhat_satellite to be false.
>     So there is a slight difference between these to ways of writing it.

I see. I was not aware getting undefined value from API is a possiblity over 
here. Usually that's not the case anywhere else in ambari APIs. can we do any 
one of the following so that someone else does not refactor the code 
incorrectly in the future:
1) Leave 1-line comment over there explaining this.
2) Add a unit test. so it correctly breaks if someone tries to refactor it in 
any other way
3) Write it in a way it's clear that we are accounting for undefined values: 
var ambariManagedVersion = 
item.operating_systems[0].OperatingSystems.ambari_managed_repositories; 
stack.use_redhat_satellite = !Em.none(ambariManagedVersion) && 
!ambariManagedVersion


- Jaimin


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


On April 29, 2016, 11:12 p.m., Zhe (Joe) Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46460/
> -----------------------------------------------------------
> 
> (Updated April 29, 2016, 11:12 p.m.)
> 
> 
> Review request for Ambari, Jaimin Jetly, Nate Cole, Richard Zang, Xi Wang, 
> and Yusaku Sako.
> 
> 
> Bugs: AMBARI-16002
>     https://issues.apache.org/jira/browse/AMBARI-16002
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Provide RedHat satellite option. It can be used wherever we show redhat6 
> urls, and applicable to redhat6/redhat7. For example, registering repos, etc.
> The API call to update links when using VDF can specify the new property. In 
> order to make it more generic (and default true), it is called 
> "ambari_managed_repositories". API looks like:
> PUT /api/v1/stacks/HDP/versions/2.3/repository_versions/2
> {
>   "RepositoryVersions" : {
>     "id" : 2
>   },
>   "operating_systems" : [
>     {
>       "OperatingSystems" : {
>         "ambari_managed_repositories": false,
>         "os_type" : "redhat6",
>         "repository_version_id" : 2,
>         "stack_name" : "HDP",
>         "stack_version" : "2.3"
>       },
>       "repositories" : [
>         {
>           "Repositories" : {
>             "base_url" : "http://repos.ambari.apache.org/hdp/HDP-2.3.4.14-4";,
>             "repo_id" : "HDP-2.3",
>             "repo_name" : "HDP"
>           }
>         },
>         {
>           "Repositories" : {
>             "base_url" : 
> "http://repos.ambari.apache.org/hdp/HDP-UTILS-1.1.0.20";,
>             "repo_id" : "HDP-UTILS-1.1.0.20",
>             "repo_name" : "HDP-UTILS"
>           }
>         }
>       ]
>     }
>   ]
> }
> 
> 
> Diffs
> -----
> 
>   ambari-web/app/controllers/installer.js 39a7812 
>   ambari-web/app/mappers/stack_mapper.js 4f73612 
>   ambari-web/app/models/stack.js 992a79c 
>   ambari-web/app/templates/wizard/step1.hbs 0476580 
>   ambari-web/app/utils/ajax/ajax.js 83914a7 
>   
> ambari-web/app/views/main/admin/stack_upgrade/upgrade_version_column_view.js 
> 168e8db 
> 
> Diff: https://reviews.apache.org/r/46460/diff/
> 
> 
> Testing
> -------
> 
> Modified unit test.
> Local ambari-web test passed.
> 25660 tests complete (24 seconds)
> 154 tests pending
> Manual testing done.
> 
> 
> Thanks,
> 
> Zhe (Joe) Wang
> 
>

Reply via email to