• 
      

    Rework Perforce repository info checks for SSL support and compatibility.

    Review Request #11029 — Created May 19, 2020 and submitted

    Information

    RBTools
    release-2.0.x
    88c15b2...

    Reviewers

    When Perforce looks up repository information, it fails to account for
    SSL variations for repositories (those with a ssl: prefix). This
    prevents some types of repositories from being found if performing a
    scan based on paths, rather than a direct name lookup.

    Now, the computed repository information factors in "Broker encryption"
    and "Server encryption" fields, and uses those to include
    ssl:-prefixed repository paths in the list of paths to check. These
    are chosen as a priority over non-SSL paths.

    As part of this, the logic for computing this information has been
    reordered and cleaned up. We now look up (and bail early on) the easy
    stuff, and save the more expensive stuff (hostname alias resolution) to
    the end.

    The hostname alias resolution has also been simplified. If there's an
    issue looking up aliases, we no longer bail early from building up a
    list of hostname candidates. Instead, we only avoid adding aliases to
    the list. This is mostly useful for unit tests, but also helps keep the
    list of possible Perforce repositories consistent even when alias
    resolution fails.

    Unit tests passed.

    Tested with a local server deployment. Verified it was adding ssl:
    prefixes as possibilities when seeing Broker encryption: encrypted
    or Server encryption: encrypted fields in p4 info.

    Description From Last Updated

    This needs a period.

    david david

    While theoretically a hair less efficient, I this this would read a lot more cleanly as: server_address = p4_info.get('Broker address') …

    david david

    E303 too many blank lines (2)

    reviewbot reviewbot

    E303 too many blank lines (2)

    reviewbot reviewbot
    Checks run (1 failed, 1 succeeded)
    flake8 failed.
    JSHint passed.

    flake8

    david
    1. 
        
    2. rbtools/clients/perforce.py (Diff revision 1)
       
       
      Show all issues

      This needs a period.

    3. rbtools/clients/perforce.py (Diff revision 1)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      While theoretically a hair less efficient, I this this would read a lot more cleanly as:

      server_address = p4_info.get('Broker address')
      encryption_state = p4_info.get('Broker encryption')
      
      if not server_address:
          server_address = p4_info.get('Server address')
          encryption_state = p4_info.get('Server encryption')
      
      if not server_address:
          return None
      
      use_ssl = (encryption_state == 'encrypted')
      
      1. How about meeting in the middle:

        server_address = p4_info.get('Broker address')
        
        if server_address:
            encryption_state = p4_info.get('Broker encryption')
        else:
            server_address = p4_info.get('Server address')
            encryption_state = p4_info.get('Server encryption')
        
        if server_address is None:
            return None
        
        use_ssl = (encryption_state == 'encrypted')
        
      2. Sure.

    4. 
        
    chipx86
    Review request changed
    Change Summary:
    • Cleaned up the logic around determining the SSL setup.
    • Added a missing period to a comment.
    Branch:
    release-1.0.x
    release-2.0.x
    Commit:
    41325a46ad32a43de095141dc3e1427599aa88fc
    ac420aa66d5681ec33cfdbde93543a4a606880bb

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    chipx86
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-2.0.x (67f2b23)