Update markbind deploy success log message#2852
Update markbind deploy success log message#2852yihao03 wants to merge 8 commits intoMarkBind:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2852 +/- ##
==========================================
+ Coverage 71.90% 72.02% +0.11%
==========================================
Files 132 132
Lines 7362 7363 +1
Branches 1637 1577 -60
==========================================
+ Hits 5294 5303 +9
+ Misses 1967 1960 -7
+ Partials 101 100 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates MarkBind’s deploy command output to be less misleading by reporting both the GitHub Actions workflow status URL and the expected deployed site URL, while refactoring the CLI logging into a utility and expanding unit tests around URL construction.
Changes:
- Extend deploy URL generation to return both GitHub Pages and GitHub Actions URLs.
- Add a CLI utility to log deploy results in a clearer, less “success-looking” way.
- Add unit tests for GitHub remote parsing and URL construction helpers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/Site/SiteDeployManager.ts | Introduces DeployResult and URL parsing/construction helpers; returns both GH Pages + Actions URLs from deployment URL resolution. |
| packages/core/index.ts | Re-exports DeployResult type for CLI consumption. |
| packages/cli/src/util/deploy.ts | Adds logDeployResult helper to centralize deploy logging behavior. |
| packages/cli/src/cmd/deploy.ts | Switches deploy command to use the new logging helper and updated deploy return shape. |
| packages/core/test/unit/Site/SiteDeployManager.test.ts | Adds unit tests for parsing remotes and constructing deploy URLs (including CNAME behavior). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // https://github.com/<name|org>/<repo>.git (HTTPS) | ||
| const repoNameWithExt = parts[parts.length - 1]; | ||
| const repoName = repoNameWithExt.substring(0, repoNameWithExt.lastIndexOf('.')); | ||
| const name = parts[parts.length - 2].toLowerCase(); | ||
| return { name, repoName }; | ||
| } else if (remoteUrl.startsWith(SSH_PREAMBLE)) { | ||
| // git@github.com:<name|org>/<repo>.git (SSH) | ||
| const repoNameWithExt = parts[parts.length - 1]; | ||
| const repoName = repoNameWithExt.substring(0, repoNameWithExt.lastIndexOf('.')); | ||
| const name = parts[0].substring(SSH_PREAMBLE.length); |
There was a problem hiding this comment.
parseGitHubRemoteUrl derives repoName via substring(0, lastIndexOf('.')), which becomes an empty string when the remote URL does not end with a file extension (e.g. https://github.com/user/repo). It can also truncate repo names containing dots when .git is absent. Consider stripping a trailing .git only when present (otherwise keep the full last path segment) so URL construction remains correct for common remote formats.
| // https://github.com/<name|org>/<repo>.git (HTTPS) | |
| const repoNameWithExt = parts[parts.length - 1]; | |
| const repoName = repoNameWithExt.substring(0, repoNameWithExt.lastIndexOf('.')); | |
| const name = parts[parts.length - 2].toLowerCase(); | |
| return { name, repoName }; | |
| } else if (remoteUrl.startsWith(SSH_PREAMBLE)) { | |
| // git@github.com:<name|org>/<repo>.git (SSH) | |
| const repoNameWithExt = parts[parts.length - 1]; | |
| const repoName = repoNameWithExt.substring(0, repoNameWithExt.lastIndexOf('.')); | |
| const name = parts[0].substring(SSH_PREAMBLE.length); | |
| // https://github.com/<name|org>/<repo>[.git] (HTTPS) | |
| const repoSegment = parts[parts.length - 1] || ''; | |
| const repoName = repoSegment.endsWith('.git') | |
| ? repoSegment.substring(0, repoSegment.length - 4) | |
| : repoSegment; | |
| const name = (parts[parts.length - 2] || '').toLowerCase(); | |
| if (!name || !repoName) { | |
| return null; | |
| } | |
| return { name, repoName }; | |
| } else if (remoteUrl.startsWith(SSH_PREAMBLE)) { | |
| // git@github.com:<name|org>/<repo>[.git] (SSH) | |
| const lastPart = parts[parts.length - 1] || ''; | |
| const repoName = lastPart.endsWith('.git') | |
| ? lastPart.substring(0, lastPart.length - 4) | |
| : lastPart; | |
| const ownerAndHost = parts[0] || ''; | |
| const name = ownerAndHost.substring(SSH_PREAMBLE.length).toLowerCase(); | |
| if (!name || !repoName) { | |
| return null; | |
| } |
There was a problem hiding this comment.
Notably, in the same file we have extractRepoSlug() that forces .git on the remotes. To avoid code duplication and ensure consistent logic, we shall just relax the .git restriction and call parseGitHubRemoteUrl() in extractRepoSlug()
packages/cli/src/util/deploy.ts
Outdated
| export function logDeployResult(result: DeployResult) { | ||
| if (result.ghActionsUrl) { | ||
| logger.info(`GitHub Actions deployment initiated. Check status at: ${result.ghActionsUrl}`); | ||
| } | ||
| if (result.ghPagesUrl) { | ||
| logger.info(`The website will be deployed at: ${result.ghPagesUrl}`); |
There was a problem hiding this comment.
ghPagesUrl can be a bare domain from CNAME (e.g. custom.domain.com), which is not a URL without a scheme. To match the intent of logging a deployed URL, consider normalizing ghPagesUrl for display (e.g. prefixing https:// when no scheme is present).
| export function logDeployResult(result: DeployResult) { | |
| if (result.ghActionsUrl) { | |
| logger.info(`GitHub Actions deployment initiated. Check status at: ${result.ghActionsUrl}`); | |
| } | |
| if (result.ghPagesUrl) { | |
| logger.info(`The website will be deployed at: ${result.ghPagesUrl}`); | |
| function normalizeUrlForDisplay(url: string): string { | |
| // If the URL already has a scheme (e.g. http://, https://), leave it as is. | |
| if (/^[a-zA-Z][\w+.-]*:\/\//.test(url)) { | |
| return url; | |
| } | |
| // Assume HTTPS for bare domains such as those from GitHub Pages CNAME. | |
| return `https://${url}`; | |
| } | |
| export function logDeployResult(result: DeployResult) { | |
| if (result.ghActionsUrl) { | |
| logger.info(`GitHub Actions deployment initiated. Check status at: ${result.ghActionsUrl}`); | |
| } | |
| if (result.ghPagesUrl) { | |
| const ghPagesDisplayUrl = normalizeUrlForDisplay(result.ghPagesUrl); | |
| logger.info(`The website will be deployed at: ${ghPagesDisplayUrl}`); |
There was a problem hiding this comment.
Wondering, does the changes account for enough flexibility in supporting deployment through other CI platforms (e.g. Travis, AppVeyor CI, Circle CI), and also deploying to
netlify?
https://markbind.org/userGuide/deployingTheSite.html#deploying-to-github-pages
Edit: Will take a look at this again
- Disregard the previous comment above ^, I misunderstood as the
deploycommand is by default configured for GitHub pages, and not meant to be flexible for other CI platforms or deployments.
|
Todos:
Otherwise great work on this 👍 |
|
Maybe can tackle #1575 at the same time in this PR, or close it if it already fixes it |
now contructs slug using parseGitHubRemoteUrl
yihao03
force-pushed
the
feat/update-deploy-log
branch
from
83f4ea7 to
1f6b153
Compare
What is the purpose of this pull request?
Overview of changes:
Addresses #2849.
Now update the log message to point to URL of the GitHub Actions and the deployed site URL
Anything you'd like to highlight/discuss:
Testing instructions:
Deploy a website using markbind and observe the log message.
Proposed commit message: (wrap lines at 72 characters)
Update deploy log message to include GitHub Actions URL and deployed site URL
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):