From 80964e63c746eebb84e3cceeb7cda94164363f60 Mon Sep 17 00:00:00 2001 From: Sam Perman Date: Wed, 19 Aug 2026 07:06:41 -0400 Subject: [PATCH] address PR review feedback on the Connect Cloud auth refactor Reject Connect-only deploy options for Connect Cloud. Reject --draft so the deploy step no longer cites a Connect version for a target that has no draft step at any version. Skip rewriting the store file when a save would not change it. DataStore._set always calls save(), so a Connect Cloud token refresh backed by the system keyring rewrote servers.json on every refresh with contents identical to what was already there, Guard the response status against None before comparing it, since a failed connection produces a response carrying only the exception. Trim the changelog entry, and rewrite two comments review found hard to follow. --- docs/CHANGELOG.md | 16 +------ rsconnect/api.py | 15 ++++--- rsconnect/certificates.py | 9 +--- rsconnect/metadata.py | 40 +++++++++++++----- rsconnect/validation.py | 50 +++++++++++++++++++--- tests/test_connect_cloud.py | 84 ++++++++++++++++++++++++++++++++++--- tests/test_metadata.py | 26 ++++++++++++ 7 files changed, 190 insertions(+), 50 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 6c0b4604..5f53b154 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -8,21 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased - Posit Connect Cloud is now a supported deployment target, alongside Posit - Connect and shinyapps.io, mirroring the R rsconnect package's support. - Register an account with `rsconnect add -n --connect-cloud - -A `, which verifies the account exists and that you can publish to - it. Authentication is an interactive browser login by default; for CI and - other non-interactive use, pass a service account credential with - `--client-id`/`--client-secret` (or the `CONNECT_CLOUD_CLIENT_ID` and - `CONNECT_CLOUD_CLIENT_SECRET` environment variables). Tokens are refreshed - automatically. Deploy with any `rsconnect deploy` subcommand by passing - `--connect-cloud` (or `-s connect.posit.cloud`) with `-A ` (or the - `CONNECT_CLOUD_ACCOUNT` environment variable; a `SHINYAPPS_ACCOUNT` variable - exported for shinyapps.io is ignored here), or `-n ` for a saved - credential — which publishes to the account it was saved with, or to another - account of the same login when `-A` is given as well. Supported content types: - Shiny (Python and R), Streamlit, Dash, Bokeh, Jupyter notebooks, Quarto, - R Markdown, and static content. + Connect and shinyapps.io. - `rsconnect add` now reports invalid option combinations and unreadable certificate files as plain error messages. Previously these surfaced as raw Python tracebacks. diff --git a/rsconnect/api.py b/rsconnect/api.py index 7aa3b3d1..aa0bcd40 100644 --- a/rsconnect/api.py +++ b/rsconnect/api.py @@ -177,7 +177,7 @@ def handle_bad_response(self, response: HTTPResponse | T, is_httpresponse: bool response.json_data["error"], ) raise RSConnectException(error, status=response.status) - if response.status < 200 or response.status > 299: + if response.status is None or response.status < 200 or response.status > 299: raise RSConnectException( "Received an unexpected response from %s (calling %s): %s %s" % ( @@ -2254,6 +2254,10 @@ def should_deploy_as_draft(self, draft: bool, no_verify: bool) -> bool: With ``--no-verify`` we activate immediately. """ if draft: + if not isinstance(self.client, RSConnectClient): + # Neither Connect Cloud nor shinyapps.io has a draft step at any version, + # so the version below would send the reader looking for one. + raise RSConnectException("Deploying as a draft is only supported by Posit Connect.") if not self.supports_verify_before_activate: # We can't honor --draft without the activate field: silently activating # would be the opposite of what the user asked for, so fail loudly. @@ -3196,11 +3200,10 @@ def update_content( ) -> ConnectCloudContent: """Update content, optionally minting a fresh revision to upload into. - `content_type` and `primary_file` are always sent alongside `app_mode`: - the API only recomputes `app_mode` when one of them is present in the - override set, and the stored content type would otherwise survive a - redeploy that changes what kind of content this is (--app-id pointing at - content of another type). + `revision_overrides` is a partial set: any field left out keeps the value + already stored on the content. All three go every time so a redeploy that + changes what kind of content this is (--app-id pointing at content of + another type) does not keep the old content type. `secrets` replaces the content's whole set, and `title` overwrites the stored one, so both are omitted from the request entirely when None: a diff --git a/rsconnect/certificates.py b/rsconnect/certificates.py index 95b98714..a57be574 100644 --- a/rsconnect/certificates.py +++ b/rsconnect/certificates.py @@ -23,13 +23,8 @@ def read_certificate_file(location: str) -> str | bytes: path = Path(location) - # Readability is judged before the suffix so a bad path reports as the real - # problem: the file type of a file that does not exist is beside the point. - # Both are operational errors: the CLI no longer checks existence at parse - # time (the certificate only applies once the target is known), so this is - # where a bad path surfaces. is_file() sits inside the handler because it - # raises OSError itself when the path's metadata cannot be read, e.g. - # through a permission-denied directory. + # is_file() is inside the try because it raises OSError itself when the path's + # metadata cannot be read, e.g. through a permission-denied directory. try: if not path.is_file(): raise RSConnectException("The certificate file %s could not be read: no such file." % location) diff --git a/rsconnect/metadata.py b/rsconnect/metadata.py index f08783dc..eacd1154 100644 --- a/rsconnect/metadata.py +++ b/rsconnect/metadata.py @@ -245,6 +245,21 @@ def save_to(self, path: str, data: bytes, open: Callable[..., BufferedWriter] = f.write(data) self._real_path = path + def _already_holds(self, path: str, data: bytes) -> bool: + """ + Whether this store was loaded from `path` and that file already holds `data`. + + Restricted to the path we read so a store loaded from its secondary + location still migrates to the primary one on the next save. + """ + if self._real_path != path or not exists(path): + return False + try: + with open(path, "rb") as f: + return f.read() == data + except OSError: + return False + # noinspection PyShadowingBuiltins def save(self, open: Callable[..., BufferedWriter] = open): """ @@ -252,19 +267,22 @@ def save(self, open: Callable[..., BufferedWriter] = open): The app directory is tried first. If that fails, then we write to the global config location. + + A save that would not change the file is skipped. """ data = json.dumps(self._data, indent=4).encode("utf-8") - try: - makedirs(self._primary_path) - self.save_to(self._primary_path, data, open) - except OSError: - if not self._secondary_path: - raise - makedirs(self._secondary_path) - self.save_to(self._secondary_path, data, open) - - if self._chmod and self._real_path is not None: - os.chmod(self._real_path, 0o600) + if not self._already_holds(self._primary_path, data): + try: + makedirs(self._primary_path) + self.save_to(self._primary_path, data, open) + except OSError: + if not self._secondary_path: + raise + makedirs(self._secondary_path) + self.save_to(self._secondary_path, data, open) + + if self._chmod and self._real_path is not None: + os.chmod(self._real_path, 0o600) class ServerDataDict(TypedDict): diff --git a/rsconnect/validation.py b/rsconnect/validation.py index 43791b9d..ce482462 100644 --- a/rsconnect/validation.py +++ b/rsconnect/validation.py @@ -60,6 +60,38 @@ def _get_present_options( return result +# Deploy options that configure Posit Connect features Connect Cloud does not +# have. +_CONNECT_ONLY_DEPLOY_OPTIONS: dict[str, str] = { + "image": "-I/--image", + "disable_env_management": "--disable-env-management", + "env_management_py": "--disable-env-management-py", + "env_management_r": "--disable-env-management-r", + "draft": "--draft", + "metadata": "--metadata", +} + + +def _typed_connect_only_deploy_options(ctx: Optional[click.Context]) -> list[str]: + """The Connect-only deploy options this command line passed. + + Judged by parameter source rather than by value: --disable-env-management-py + inverts to False when given, and the --disable-env-management shorthand fills + in the per-language parameters without being their source. + + Only options count. `environment add` takes a positional IMAGE argument, whose + parameter is also named `image` and is not this option. + """ + if ctx is None: + return [] + option_names = {param.name for param in ctx.command.params if isinstance(param, click.Option)} + return [ + label + for name, label in _CONNECT_ONLY_DEPLOY_OPTIONS.items() + if name in option_names and get_parameter_source_name_from_ctx(name, ctx) == "COMMANDLINE" + ] + + def validate_connect_cloud_incompatible_options( ctx: Optional[click.Context], api_key: Optional[str], @@ -69,12 +101,12 @@ def validate_connect_cloud_incompatible_options( ): """Reject options that have no meaning on Posit Connect Cloud. - validate_connection_options performs the same checks, but only when Connect - Cloud is selected by flag or URL; a saved nickname or default server is only - identified as Connect Cloud after store resolution, so the executor calls - this afterwards. Environment-sourced values are ignored for the same reason - as elsewhere: a CONNECT_API_KEY exported for another target is just the - CI environment, not a request to use it here. + validate_connection_options repeats the credential and SPCS checks, but only + when Connect Cloud is selected by flag or URL; a saved nickname or default + server is only identified as Connect Cloud after store resolution, so the + executor calls this afterwards. Environment-sourced values are ignored for + the same reason as elsewhere: a CONNECT_API_KEY exported for another target + is just the CI environment, not a request to use it here. """ present_connect_options = _get_present_options( {"-k/--api-key": api_key, "-i/--insecure": insecure, "-c/--cacert": cacert}, @@ -92,6 +124,12 @@ def validate_connect_cloud_incompatible_options( if present_spcs_options: raise RSConnectException( f"SPCS options ({', '.join(present_spcs_options)}) may not be passed \ +alongside Posit Connect Cloud. See command help for further details." + ) + connect_only_deploy_options = _typed_connect_only_deploy_options(ctx) + if connect_only_deploy_options: + raise RSConnectException( + f"Posit Connect options ({', '.join(connect_only_deploy_options)}) may not be passed \ alongside Posit Connect Cloud. See command help for further details." ) diff --git a/tests/test_connect_cloud.py b/tests/test_connect_cloud.py index 9a542160..3b9bd30e 100644 --- a/tests/test_connect_cloud.py +++ b/tests/test_connect_cloud.py @@ -30,12 +30,14 @@ from rsconnect.metadata import AppStore, ServerData, ServerStore from rsconnect.models import AppModes from rsconnect.oauth import InvalidClientError, InvalidGrantError +from rsconnect import validation from rsconnect.validation import validate_connection_options from .utils import failing_keyring ENV = ParameterSource.ENVIRONMENT TYPED = ParameterSource.COMMANDLINE +DEFAULT = ParameterSource.DEFAULT class TestConnectCloudEnvironments(unittest.TestCase): @@ -337,8 +339,13 @@ def _json_body(request): def _ctx(**sources: ParameterSource) -> click.Context: - """A click context recording where each named parameter's value came from.""" - ctx = click.Context(click.Command("deploy")) + """A click context recording where each named parameter's value came from. + + Each name is declared as an option, since validation distinguishes options + from same-named arguments. + """ + params: list[click.Parameter] = [click.Option(["--%s" % param.replace("_", "-")]) for param in sources] + ctx = click.Context(click.Command("deploy", params=params)) for param, source in sources.items(): ctx.set_parameter_source(param, source) # pyright: ignore[reportAttributeAccessIssue] return ctx @@ -665,9 +672,9 @@ def _update_content(self, **kwargs): return _json_body(httpretty.last_request()) @httpretty.activate(verbose=True, allow_net_connect=False) - def test_update_content_sends_primary_file_with_app_mode(self): - # The API only recomputes app_mode when content_type or primary_file is - # in the override set; sending app_mode alone would persist it verbatim. + def test_update_content_sends_the_whole_revision_override_set(self): + # Omitted overrides keep the content's stored value, so all three go every + # time or a redeploy of a different kind of content keeps the old ones. body = self._update_content() self.assertEqual( body["revision_overrides"], @@ -2985,6 +2992,73 @@ def test_remove_by_url_with_a_single_entry_removes_it(self): self.assertEqual(store.get_all_servers(), []) +class TestConnectOnlyDeployOptions(unittest.TestCase): + """Deploy options that configure Posit Connect features Connect Cloud does not + have. Connect Cloud ignores them, so they are rejected rather than accepted and + dropped.""" + + OPTIONS = { + "image": "-I/--image", + "disable_env_management": "--disable-env-management", + "env_management_py": "--disable-env-management-py", + "env_management_r": "--disable-env-management-r", + "draft": "--draft", + "metadata": "--metadata", + } + + def test_each_is_rejected_with_the_flag(self): + for param, label in self.OPTIONS.items(): + with self.subTest(param): + with self.assertRaises(RSConnectException) as context: + _setup_remote_server(ctx=_ctx(**{param: TYPED}), account_name="acme", use_connect_cloud=True) + message = str(context.exception) + self.assertIn(label, message) + self.assertIn("may not be passed alongside Posit Connect Cloud", message) + + def test_each_is_rejected_for_a_saved_cloud_nickname(self): + # A nickname is only known to name a Connect Cloud credential after the + # store lookup, which is why this is checked in the executor. + for param, label in self.OPTIONS.items(): + with self.subTest(param): + with self.assertRaises(RSConnectException) as context: + _setup_remote_server( + ctx=_ctx(**{param: TYPED}), resolve=_cloud_entry(connect_cloud_access_token="at"), name="cloud" + ) + self.assertIn(label, str(context.exception)) + + def test_defaulted_options_are_accepted(self): + # --disable-env-management-py inverts to False when given and the shorthand + # sets the same parameters without being their source, so what the user + # typed can only be read from the parameter source. + _cloud_server(ctx=_ctx(image=DEFAULT, env_management_py=DEFAULT), account_name="acme", use_connect_cloud=True) + + def test_a_connect_target_still_accepts_them(self): + store = ServerStore(base_dir=tempfile.mkdtemp()) + store.set("prod", "https://connect.example.com", api_key="key") + executor = _setup_remote_server(ctx=_ctx(image=TYPED), store=store, name="prod") + self.assertIsInstance(executor.remote_server, api.RSConnectServer) + + def test_draft_at_the_deploy_step_does_not_cite_a_connect_version(self): + # The CLI rejects --draft before this, but a programmatic caller has no + # click context to judge, and a Connect version says nothing to a target + # with no draft step. + executor = RSConnectExecutor.__new__(RSConnectExecutor) + executor.client = mock.Mock(spec=ConnectCloudClient) + with self.assertRaises(RSConnectException) as context: + executor.should_deploy_as_draft(draft=True, no_verify=False) + message = str(context.exception) + self.assertIn("only supported by Posit Connect", message) + self.assertNotIn("2025.06.0", message) + + def test_a_same_named_argument_is_not_one_of_these_options(self): + # `environment add` takes a positional IMAGE, so a Connect Cloud nickname + # must reach that command's own "requires a Posit Connect server" error + # rather than be told it passed -I/--image. + command = cli.commands["environment"].commands["add"] + with command.make_context("add", ["my-image:1.0", "-n", "cloud"]) as ctx: + self.assertEqual(validation._typed_connect_only_deploy_options(ctx), []) + + class TestConnectCloudFindsSavedCredentialsByUrl(unittest.TestCase): """`--connect-cloud` and `-s connect.posit.cloud` must find a saved server. diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 849c1126..6b9ce52b 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -1,3 +1,4 @@ +import os import shutil import tempfile from os.path import exists, join @@ -253,6 +254,31 @@ def test_remove_default_clears_it(self): self.server_store.remove_by_name("foo") self.assertIsNone(self.server_store.get_default()) + def test_save_skips_rewrite_when_file_unchanged(self): + def fail_open(path_to_open, mode, *args, **kw): + self.fail("rewrote %s when nothing had changed" % path_to_open) + + self.server_store.save(fail_open) + + def test_save_rewrites_when_data_changed(self): + writes = [] + + def recording_open(path_to_open, mode, *args, **kw): + writes.append(path_to_open) + return open(path_to_open, mode, *args, **kw) + + del self.server_store._data["foo"] + self.server_store.save(recording_open) + + self.assertEqual(writes, [self.server_store_path]) + self.assertIsNone(ServerStore(base_dir=self.tempDir).get_by_name("foo")) + + def test_save_rewrites_when_file_removed(self): + os.remove(self.server_store_path) + self.server_store.save() + + self.assertEqual(len(ServerStore(base_dir=self.tempDir).get_all_servers()), 5) + class TestAppMetadata(TestCase): def setUp(self):