ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Nettleton" <rnettle...@hortonworks.com>
Subject Re: Review Request 39687: Allow use passwords references in custom actions
Date Mon, 02 Nov 2015 21:43:41 GMT

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

Ship it!


Overall, the patch looks fine to me, just a few minor issues that should be resolved first.


Thanks for providing the overall context information I requested earlier regarding secret
references and how they are used in Ambari.


ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java
(line 47)
<https://reviews.apache.org/r/39687/#comment163041>

    Minor Issue:
    
    In general, we try to use explicit references for imports. 
    
    I'd recommend changing this import statement to explicitly list each type imported.



ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java (line 442)
<https://reviews.apache.org/r/39687/#comment163042>

    Minor issue:
    
    If this whitespace change isn't required for this patch, I'd recommend removing it.  
    
    Thanks.



ambari-server/src/main/java/org/apache/ambari/server/state/StackInfo.java (line 74)
<https://reviews.apache.org/r/39687/#comment163043>

    Minor issue:
    
    I'd recommend making this field private, unless there is a specific reason not to.



ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java (line
2935)
<https://reviews.apache.org/r/39687/#comment163044>

    Why was the "synchronized" keyword removed?  
    
    It looks like this method was moved into the StackInfo class, but I would imagine that
the concurrency requirements would be the same for protecting this data from concurrent access.



ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java
(line 47)
<https://reviews.apache.org/r/39687/#comment163045>

    Minor issue:
    
    Please use explicit imports here.


- Robert Nettleton


On Nov. 2, 2015, 5:24 p.m., Vitalyi Brodetskyi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39687/
> -----------------------------------------------------------
> 
> (Updated Nov. 2, 2015, 5:24 p.m.)
> 
> 
> Review request for Ambari, Dmitro Lisnichenko, Dmytro Sen, Robert Nettleton, and Sid
Wagle.
> 
> 
> Bugs: AMBARI-13582
>     https://issues.apache.org/jira/browse/AMBARI-13582
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Added ability to use secret references in custom actions; /api/v1/blueprints and /api/v1/clusters/cl?format=blueprint
returns masked passwords.
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariActionExecutionHelper.java
d834731 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
5f432ef 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java 625ebc6

>   ambari-server/src/main/java/org/apache/ambari/server/controller/ConfigurationResponse.java
3ed9306 
>   ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java
fa355fa 
>   ambari-server/src/main/java/org/apache/ambari/server/stack/StackModule.java def33f0

>   ambari-server/src/main/java/org/apache/ambari/server/state/StackInfo.java e3ac3e0 
>   ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java
279b31f 
>   ambari-server/src/main/java/org/apache/ambari/server/topology/BlueprintValidatorImpl.java
9e8f163 
>   ambari-server/src/main/java/org/apache/ambari/server/utils/SecretReference.java 2b1aeae

>   ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java
c6170e4 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java
369bf02 
>   ambari-server/src/test/java/org/apache/ambari/server/topology/BlueprintImplTest.java
2199247 
>   ambari-server/src/test/resources/stacks/HDP/2.0.6/services/YARN/configuration/yarn-site.xml
1c02e86 
> 
> Diff: https://reviews.apache.org/r/39687/diff/
> 
> 
> Testing
> -------
> 
> mvn clean test
> 
> 
> Thanks,
> 
> Vitalyi Brodetskyi
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message