Skip to content

decouple connect-specific publishing code from generic publishing code #2703

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

Open
wants to merge 12 commits into
base: schema-v4
Choose a base branch
from

Conversation

mslynch
Copy link
Collaborator

@mslynch mslynch commented Jul 11, 2025

Intent

Refactor to decouple publishing and Connect code.

Type of Change

    • Bug Fix
    • New Feature
    • Breaking Change
    • Documentation
    • Refactor
    • Tooling

Approach

I extracted the connect-specific code from publish into its own module. Later I'll add a similar module with a ServerPublisher to publish to Connect Cloud.

I wouldn't be surprised to encounter more code in need of refactoring, but this should at least be a good starting point to write the integration.

User Impact

None (hopefully)

Automated Tests

No new tests needed.

Directions for Reviewers

No special steps - automated test should still pass.

Checklist

  • I have updated CHANGELOG.md to cover notable changes. - n/a

@mslynch mslynch changed the base branch from main to schema-v4 July 11, 2025 17:23
@mslynch mslynch requested a review from Copilot July 11, 2025 17:58
Copilot

This comment was marked as outdated.

@mslynch mslynch requested a review from Copilot July 14, 2025 15:50
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the publishing flow by extracting all Connect-specific logic into a new connect subpackage and introducing a generic ServerPublisher interface, allowing the core publish package to handle high-level orchestration.

  • Extracted Connect logic (create, update, deploy, validate, env-vars, bundle upload) into internal/publish/connect
  • Introduced PublishHelper for shared deployment record operations and refactored defaultPublisher to use it
  • Updated tests to wire in PublishHelper, replaced old publishWithClient calls with PublishDirectory, and adapted mocking

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/services/api/post_deployment_cancel.go Updated handler to call new CancelDeployment signature
internal/publish/server_publisher.go Added factory to create ServerPublisher by server type
internal/publish/r_package_descriptions_test.go Injected PublishHelper into R package description tests
internal/publish/r_functional_test.go Replaced publishWithClient with PublishDirectory, stubbed client
internal/publish/publishhelper/helper.go Introduced PublishHelper and ContentInfo for common operations
internal/publish/publish_test.go Updated mocks for renamed capabilitiesErr and integrated helper
internal/publish/publish.go Refactored PublishDirectory into doPublish, removed direct Connect code
internal/publish/interpreters.go Extracted interpreter setup into configureInterpreters
internal/publish/connect/validate.go Moved validation into ServerPublisher.validateContent
internal/publish/connect/update_content.go Moved update logic into ServerPublisher.updateContent
internal/publish/connect/set_env_vars_test.go Adapted tests to use ServerPublisher for environment variables
internal/publish/connect/set_env_vars.go Moved setEnvVars into ServerPublisher
internal/publish/connect/server_publisher.go Implemented ServerPublisher orchestration methods
internal/publish/connect/preflight_checks.go Moved PreFlightChecks into ServerPublisher
internal/publish/connect/deploy_bundle.go Moved deployBundle into ServerPublisher
internal/publish/connect/create_deployment.go Moved CreateDeployment into ServerPublisher
internal/publish/connect/bundle.go Added bundle upload and record write in ServerPublisher
internal/publish/bundle.go Refactored createBundle to return the bundle file
Comments suppressed due to low confidence (3)

internal/services/api/post_deployment_cancel.go:20

  • The handler still declares a localid path parameter but never uses it—either reintroduce its use in CancelDeployment or remove the route variable and related references for consistency.
		name := mux.Vars(req)["name"]

internal/publish/connect/validate.go:25

  • This line uses util.GetDirectURL but the util package isn't imported—add "github.com/posit-dev/publisher/internal/util" to the imports to prevent a compile error.
		DirectURL: util.GetDirectURL(c.Account.URL, c.Target.ID),

internal/publish/publish.go:205

  • [nitpick] Currently logAppInfo only prints URLs when publishingErr is non-nil; successful publishes won't display any deployment URLs—consider updating it to also show info on success for better UX.
	p.logAppInfo(os.Stderr, p.log, err)

@mslynch mslynch requested a review from rodriin July 14, 2025 15:54
@mslynch mslynch marked this pull request as ready for review July 14, 2025 15:54
@marcosnav
Copy link
Collaborator

@mslynch I'm in the process of reviewing the PR, so far the approach you are taking looks great, I think it is a great idea to decouple the publishing process and have multiple publishing providers, if you will.

I haven't finished taking a look but there are a couple things I already noticed and want to bring up:

  1. It'd be great to include unit tests for the code under the new /internal/publish/* files. I understand some of these are being mostly re-organized and that we didn't have tests prior to this changes, so it is not totally fair to come asking for those now 😅 ... but with the increase of added functionality, I think skipping tests should not be an option anymore.
  2. Not suggesting to be addressed in this PR. We should start to figure out a way to have integration tests too. Maybe there have been conversations within the hosted team about this already, for now thinking here out loud, new workflows are being introduced for which we are needing checks as insurance practice for future changes. Happy to join your team on conversations about this to have better testing practices in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants