Merge pull request #518 from rstudio/sagerb-conflicting-params

Improve the error output for conflicting params and provide better option help describing environment variables used

Author: sagerb

CHANGELOG.md

@@ -4,6 +4,20 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## Pending Next Release
+
+### Added
+- Added the name of the environment variables to the help output for those options that
+  use environment variables as a default value.
+
+### Changed
+- Improved the error and warning outputs when options conflict by providing the source 
+  from which the values have been determined. This allows for faster resolution of issues
+  when combinations of stored credentials, environment variables and command line options
+  are used.
+- Updated verbose mode to output the source of all options being used when processing the
+  CLI command.
+
 ## [1.21.0] - 2023-10-26
 
 ### Fixed

integration-testing/docker/cypress.Dockerfile

@@ -1,7 +1,8 @@
 FROM cypress/included:12.7.0
 
 RUN apt-get update -y && apt-get install -y --no-install-recommends \
-    jq
+    jq \
+    curl
 
 RUN mkdir -p /libs-cypress && \
     curl -fsSL https://github.com/casey/just/releases/download/1.1.2/just-1.1.2-x86_64-unknown-linux-musl.tar.gz \
@@ -10,4 +11,4 @@ RUN mkdir -p /libs-cypress && \
 ENV ADMIN_API_KEY=${ADMIN_API_KEY}
 ENV PATH=$PATH:/libs-cypress
 CMD cypress run --browser chrome --env api_key=${ADMIN_API_KEY}
-# CMD run cypress:open --env api_key=${ADMIN_API_KEY}
+# CMD run cypress:open --env api_key=${ADMIN_API_KEY}

pyproject.toml

@@ -13,6 +13,7 @@ dependencies = [
     "pip>=10.0.0",
     "semver>=2.0.0,<3.0.0",
     "pyjwt>=2.4.0",
+    "click>=8.0.0",
 ]
 
 dynamic = ["version"]
@@ -33,9 +34,9 @@ test = [
     "pytest-cov",
     "pytest-mypy",
     "pytest",
-    "setuptools_scm[toml]",
+    "setuptools>=61",
+    "setuptools_scm[toml]>=3.4",
     "twine",
-    "types-click",
     "types-Flask",
     "types-six",
 ]

requirements.txt

@@ -10,7 +10,7 @@ wheel
 
 # project.dependencies
 six>=1.14.0
-click>=7.0.0
+click>=8.0.0
 pip>=10.0.0
 semver>=2.0.0,<3.0.0
 pyjwt>=2.4.0
@@ -29,6 +29,5 @@ pytest-mypy
 pytest
 setuptools_scm[toml]
 twine
-types-click
 types-Flask
 types-six

rsconnect/api.py

@@ -19,6 +19,7 @@
 import re
 from warnings import warn
 import gc
+import click
 
 from . import validation
 from .certificates import read_certificate_file
@@ -389,6 +390,7 @@ def output_task_log(task_status, last_status, log_callback):
 class RSConnectExecutor:
     def __init__(
         self,
+        ctx: click.Context = None,
         name: str = None,
         url: str = None,
         api_key: str = None,
@@ -406,7 +408,9 @@ def __init__(
         self.reset()
         self._d = kwargs
         self.logger = logger
+        self.ctx = ctx
         self.setup_remote_server(
+            ctx=ctx,
             name=name,
             url=url or kwargs.get("server"),
             api_key=api_key,
@@ -442,8 +446,31 @@ def drop_context(self):
         gc.collect()
         return self
 
+    def output_overlap_header(self, previous):
+        if self.logger and not previous:
+            self.logger.warning(
+                "\nConnect detected CLI commands and/or environment variables that overlap with stored credential.\n"
+            )
+            self.logger.warning(
+                "Check your environment variables (e.g. CONNECT_API_KEY) to make sure you want them to be used.\n"
+            )
+            self.logger.warning(
+                "Credential parameters are taken with the following precedence: stored > CLI > environment.\n"
+            )
+            self.logger.warning(
+                "To ignore an environment variable, override it in the CLI with an empty string (e.g. -k '').\n\n"
+            )
+            return True
+
+    def output_overlap_details(self, cli_param, previous):
+        new_previous = self.output_overlap_header(previous)
+        sourceName = validation.get_parameter_source_name_from_ctx(cli_param, self.ctx)
+        self.logger.warning(f">> stored {cli_param} value overrides the {cli_param} value from {sourceName}\n")
+        return new_previous
+
     def setup_remote_server(
         self,
+        ctx: click.Context,
         name: str = None,
         url: str = None,
         api_key: str = None,
@@ -455,6 +482,7 @@ def setup_remote_server(
         secret: str = None,
     ):
         validation.validate_connection_options(
+            ctx=ctx,
             url=url,
             api_key=api_key,
             insecure=insecure,
@@ -464,45 +492,37 @@ def setup_remote_server(
             secret=secret,
             name=name,
         )
+        header_output = False
 
         if cacert and not ca_data:
             ca_data = read_certificate_file(cacert)
 
         server_data = ServerStore().resolve(name, url)
         if server_data.from_store:
             url = server_data.url
-            if (
-                server_data.api_key
-                and api_key
-                or server_data.insecure
-                and insecure
-                or server_data.ca_data
-                and ca_data
-                or server_data.account_name
-                and account_name
-                or server_data.token
-                and token
-                or server_data.secret
-                and secret
-            ) and self.logger:
-                self.logger.warning(
-                    "Connect detected CLI commands and/or environment variables that overlap with stored credential.\n"
-                )
-                self.logger.warning(
-                    "Check your environment variables (e.g. CONNECT_API_KEY) to make sure you want them to be used.\n"
-                )
-                self.logger.warning(
-                    "Credential paremeters are taken with the following precedence: stored > CLI > environment.\n"
-                )
-                self.logger.warning(
-                    "To ignore an environment variable, override it in the CLI with an empty string (e.g. -k '').\n"
-                )
+            if self.logger:
+                if server_data.api_key and api_key:
+                    header_output = self.output_overlap_details("api-key", header_output)
+                if server_data.insecure and insecure:
+                    header_output = self.output_overlap_details("insecure", header_output)
+                if server_data.ca_data and ca_data:
+                    header_output = self.output_overlap_details("cacert", header_output)
+                if server_data.account_name and account_name:
+                    header_output = self.output_overlap_details("account", header_output)
+                if server_data.token and token:
+                    header_output = self.output_overlap_details("token", header_output)
+                if server_data.secret and secret:
+                    header_output = self.output_overlap_details("secret", header_output)
+                if header_output:
+                    self.logger.warning("\n")
+
             api_key = server_data.api_key or api_key
             insecure = server_data.insecure or insecure
             ca_data = server_data.ca_data or ca_data
             account_name = server_data.account_name or account_name
             token = server_data.token or token
             secret = server_data.secret or secret
+
         self.is_server_from_store = server_data.from_store
 
         if api_key:
@@ -571,7 +591,7 @@ def validate_connect_server(
         :param url: the URL, if any, specified by the user.
         :param api_key: the API key, if any, specified by the user.
         :param insecure: a flag noting whether TLS host/validation should be skipped.
-        :param cacert: the file object of a CA certs file containing certificates to use.
+        :param cacert: the file path of a CA certs file containing certificates to use.
         :param api_key_is_required: a flag that notes whether the API key is required or may
         be omitted.
         :param token: The shinyapps.io authentication token.

rsconnect/certificates.py

@@ -21,12 +21,12 @@ def read_certificate_file(location: str):
     suffix = path.suffix
 
     if suffix in BINARY_ENCODED_FILETYPES:
-        with open(path, "rb") as file:
-            return file.read()
+        with open(path, "rb") as bFile:
+            return bFile.read()
 
     if suffix in TEXT_ENCODED_FILETYPES:
-        with open(path, "r") as file:
-            return file.read()
+        with open(path, "r") as tFile:
+            return tFile.read()
 
     types = BINARY_ENCODED_FILETYPES + TEXT_ENCODED_FILETYPES
     types = sorted(types)