Skip to content

fix(ci): move comment workflow to deploy-cd#4814

Open
riteshfyi wants to merge 23 commits intowebex:nextfrom
riteshfyi:pr
Open

fix(ci): move comment workflow to deploy-cd#4814
riteshfyi wants to merge 23 commits intowebex:nextfrom
riteshfyi:pr

Conversation

@riteshfyi
Copy link
Copy Markdown
Contributor

COMPLETES #NA

This pull request addresses

We are moving the pr-comment-workflow to deploy-cd. so we don't have to re-fetch the package changes

by making the following changes

Moves the PR release comment logic from its own workflow into deploy.yml as a final job, so it runs after the full deploy pipeline completes. Uses the increment step output directly for per-package versions instead of re-reading the changelog.

We are logging the output of the yarn increment step during npm-publish to a variable, so we are able to get the updated packages & their latest version.

Screenshot 2026-03-27 at 12 58 46 PM

Change Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • [] Tooling change
  • Internal code refactor

The following scenarios were tested

< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >

The GAI Coding Policy And Copyright Annotation Best Practices

  • GAI was not used (or, no additional notation is required)
  • Code was generated entirely by GAI
  • GAI was used to create a draft that was subsequently customized or modified
  • Coder created a draft manually that was non-substantively modified by GAI (e.g., refactoring was performed by GAI on manually written code)
  • Tool used for AI assistance (GitHub Copilot / Other - specify)
    • Github Copilot
    • Cursor - claude-4.6-opus
  • This PR is related to
    • Feature
    • Defect fix
    • Tech Debt
    • Automation
    • Refactor of workflow

I certified that

  • I have read and followed contributing guidelines
  • I discussed changes with code owners prior to submitting this pull request
  • I have not skipped any automated checks
  • All existing and new tests passed
  • I have updated the documentation accordingly

Make sure to have followed the contributing guidelines before submitting.

@riteshfyi riteshfyi requested a review from a team as a code owner March 27, 2026 07:34
@riteshfyi
Copy link
Copy Markdown
Contributor Author

Test Comment Link : riteshfyi#8 (comment)

@riteshfyi
Copy link
Copy Markdown
Contributor Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 393f72d9e3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".


comment-on-pr:
name: Comment on Released PRs
needs: [publish-npm, publish-tag]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Gate PR release comments on full deploy success

comment-on-pr is intended to run after the full deploy pipeline, but its needs only includes publish-npm and publish-tag. If publish-documentation fails, this job can still post “Your changes are now available,” which is a regression from the previous workflow_run-based flow that only ran when Deploy CD concluded successfully; this can produce incorrect release notifications for partially failed deployments.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if publish-npm, publish-tag has happened already. then the package is already in production. also publish-documentation gets skipped in some scenarios as well.

const prs = await github.rest.repos.listPullRequestsAssociatedWithCommit({
owner, repo, commit_sha: deploySha
});
const mergedPR = prs.data.find(pr => pr.merged_at);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.find() only returns the first merged PR — if multiple merged PRs are associated with this commit (back-to-back merges), the rest get silently skipped? Will this work for that case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

deploySha is unique for each pull-request, so this won't happen.

@riteshfyi
Copy link
Copy Markdown
Contributor Author

@vamshigovardhana right now we are fetching 200 comments to verify if we have any pre-exisiting comment, which we can upgrade, like a general one.
should we increase the limit?

@vamshigovardhana
Copy link
Copy Markdown
Contributor

@vamshigovardhana right now we are fetching 200 comments to verify if we have any pre-exisiting comment, which we can upgrade, like a general one. should we increase the limit?

@riteshfyi I think GitHub's API silently caps per_page at 100 -- passing 200 just returns 100 with no error, so increasing the number won't help. why don't we Use github.paginate() instead? which automatically fetches all pages regardless of how many comments exist.

Comment on lines +427 to +428
const owner = 'webex';
const repo = 'webex-js-sdk';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't these be dynamic? Why do we need to hard code these?

Copy link
Copy Markdown
Contributor Author

@riteshfyi riteshfyi Mar 31, 2026

Choose a reason for hiding this comment

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

actually it was dynamic earlier, in a review i was suggested to hard-code it, yeah, let's make it dynamic again

Comment on lines +420 to +423
for (const line of raw.split('\n')) {
const match = line.match(/^__webex_release__:\s+(.+?)\s+=>\s+(.+)$/);
if (match) packageVersions[match[1].trim()] = match[2].trim();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why can't this version match happen in the place where we extract __webex_release__: lines?

Seems to be a redundant loop

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

just a question kesava, should the job responsible for incrementing the package version, be doing this ?. i feel like it seems a little out of context for that job,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did the changes, but that method was very prone to bugs, it introduced 3 bugs which i could identify directly. even after multiple changes, the final appraoch still felt like, that was not good for production. also it was not readable also.

Comment on lines +435 to +437
const primaryPackage = packageVersions['webex']
? 'webex'
: packageEntries.filter(([p]) => p !== 'webex' && p !== 'webex-node').map(([p]) => p)[0] || packageEntries[0][0];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This condition seems to check packageVersions['webex'] is matching to the string webex itself. Would this ever match?

Copy link
Copy Markdown
Contributor Author

@riteshfyi riteshfyi Mar 31, 2026

Choose a reason for hiding this comment

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

Actually here,
it tries to find a relevant primary pacakge for the changelog url, if 'webex' is falsy or not present in the primaryPackage list, it will filter the 'webex' & 'webex-node' packages & select a random pacakge for the changelog url.

Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Image

const primaryVersion = packageVersions[primaryPackage];
const stableVersion = primaryVersion.replace(/-next\..*/, '').replace(/-[a-z]*\..*/, '');

const changelogUrl = new URL('https://web-sdk.webex.com/changelog/');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This base URL is mapped as a custom domain in the repo.

This setting sits here: https://github.com/webex/webex-js-sdk/blob/next/docs/CNAME

Can we find a way to use this so that we won't have to hardcode this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good Call.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added

packagesTable,
'Thank you for your contribution!',
'',
`_:robot: This is an automated message. For questions, please refer to the [documentation](${repoUrl}#readme)._`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should rather reword this like this:

Suggested change
`_:robot: This is an automated message. For questions, please refer to the [documentation](${repoUrl}#readme)._`
`_:robot: This is an automated message. For queries, please contact [support](https://developer.webex.com/support)._`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, indeed better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b3a8e0de9f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

echo "$OUTPUT"
VERSIONS_JSON=$(printf '%s\n' "$OUTPUT" | grep -E '^(@webex/|webex[^ ]*) => ' | \
jq -Rn '[inputs | split(" => ")] | map({(.[0]): .[1]}) | add // {}')
echo "package_versions=$VERSIONS_JSON" >> $GITHUB_OUTPUT
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Emit compact JSON before setting package_versions output

jq -Rn emits pretty-printed JSON by default, so VERSIONS_JSON contains embedded newlines here; writing it as package_versions=$VERSIONS_JSON produces a multi-line GITHUB_OUTPUT entry, which the runner treats as an invalid output format unless heredoc syntax is used. In runs where packages are incremented, this can fail publish-npm before downstream jobs can read needs.publish-npm.outputs.package_versions.

Useful? React with 👍 / 👎.

run: |
OUTPUT=$(yarn package-tools increment --packages ${{ needs.generate-package-matrix.outputs.node-recursive }} --tag ${GITHUB_REF##*/})
echo "$OUTPUT"
VERSIONS_JSON=$(printf '%s\n' "$OUTPUT" | grep -E '^(@webex/|webex[^ ]*) => ' | \
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Tolerate empty increment output when extracting package versions

This pipeline relies on grep producing at least one match, but when no packages are incremented (a case the workflow already models via hasPackages/proceed=false), grep -E exits with status 1 and the step fails under GitHub Actions' bash -eo pipefail defaults. That turns a valid no-package deploy into a hard failure in publish-npm.

Useful? React with 👍 / 👎.

@riteshfyi
Copy link
Copy Markdown
Contributor Author

riteshfyi commented Apr 1, 2026

@vamshigovardhana @mkesavan13 i'm done with my changes,
will verify the code in production with my npm account once before merge.
please let me know if you all have any more reviews.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1c9dd0bc52

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

const primaryVersion = packageVersions[primaryPackage];
const stableVersion = primaryVersion.replace(/-next\..*/, '').replace(/-[a-z]*\..*/, '');

const cnameFile = await github.rest.repos.getContent({ owner, repo, path: 'docs/CNAME' });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Guard CNAME lookup failures before composing comment body

Handle github.rest.repos.getContent({ path: 'docs/CNAME' }) failures before building the release comment, because this call is currently outside the try/catch used for comment posting. If the API call fails (for example due to a transient GitHub API error or a missing/renamed docs/CNAME on the default branch), the comment-on-pr job hard-fails and marks Deploy CD failed even after publish-npm/publish-tag have succeeded.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

should we add a fallback here?

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.

3 participants