- 
                Notifications
    You must be signed in to change notification settings 
- Fork 120
Correctly load configuration when instantiating new client without kwargs #544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| [APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anishasthana The full list of commands accepted by this bot can be found here. 
Needs approval from an approver in each of these files:
 Approvers can indicate their approval by writing  | 
| @anishasthana The  | 
        
          
                podman/client.py
              
                Outdated
          
        
      | api_kwargs["base_url"] = connection.url.geturl() | ||
| # Override configured identity, if provided in arguments | ||
| api_kwargs["identity"] = kwargs.get("identity", str(connection.identity)) | ||
| elif "base_url" not in api_kwargs: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwhonce any reason why this is technically the default case (this will always be true if api_kwargs is empty) and not the contents we get from the podman config?
f8f753e    to
    da8458c      
    Compare
  
            
          
                podman/client.py
              
                Outdated
          
        
      | elif "base_url" not in api_kwargs: | ||
| path = str(Path(get_runtime_dir()) / "podman" / "podman.sock") | ||
| api_kwargs["base_url"] = "http+unix://" + path | ||
| if not api_kwargs and "Connection" in config.attrs: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is bringing in a lot of value.
I would take the chance to rework this logic, making use of a more linear if/else chain. Here's a POC of what I mean.
wdyt @jwhonce @umohnani8 @anishasthana ?
    # Case 1: Use named connection from kwargs if specified
    if "connection" in api_kwargs:
        connection = config.services[api_kwargs.get("connection")]
        api_kwargs["base_url"] = connection.url.geturl()
        # Override configured identity, if provided in arguments
        api_kwargs["identity"] = kwargs.get("identity", str(connection.identity))
    
    # Case 2: No kwargs provided - try to load from system configuration
    elif not api_kwargs:
        default_connection_name = config.attrs["Connection"]["Default"]
        connection = config.attrs["Connection"]["Connections"][default_connection_name]
        api_kwargs["base_url"] = connection["URI"]
        api_kwargs["identity"] = connection["Identity"]
    
    # Case 3: Fallback - if no base_url is specified, use default socket path
    if "base_url" not in api_kwargs:
        path = str(Path(get_runtime_dir()) / "podman" / "podman.sock")
        api_kwargs["base_url"] = "http+unix://" + pathI would also take the chance to add some logging, and, of course, a couple more tests are needed to ensure that the config priority is maintained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried about breaking existing behaviour, but I'm happy to move to a more linear chain!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my suggestion is to enforce the behaviour with integration tests. you can proceed this way if you like:
- write tests for the current behaviour (I didn't check the test status right now), make a pr, and we merge it.
- write the new code (in this PR) and use any approach. then we'll have tests to help understand if the behaviour is breaking
we need to ensure this is tested anyway. it's quite critical for this feature :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anishasthana thanks for moving it forward
we need to ensure this is tested anyway. it's quite critical for this feature :)
I'd like to have tests for all the if/else cases in this PR. do you need help with them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @inknos, I'm in the middle of some travel so it'll be slow going on my end (as and when I have time). I'll reach out if I find myself stuck/confused!
da8458c    to
    df2e7e0      
    Compare
  
    …args The init function of PodmanClient was never loading the system configuration when no kwargs were specified. This change makes it so that we load the core data (base_url and identity) from the config file. Signed-off-by: Anish Asthana <anishasthana1@gmail.com>
df2e7e0    to
    4c537ee      
    Compare
  
    
The init function of PodmanClient was never loading the system configuration when no kwargs were specified. This change makes it so that we load the core data (base_url and identity) from the config file.