Skip to content
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

devops: Add ci warning for swizzled docusaurus components #9467

Merged
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
9fe5c60
devops: Add ci warning for swizzled docusaurus components
jNullj Aug 9, 2023
afd5505
Fix table formating
jNullj Aug 10, 2023
4e158d2
Fix order
jNullj Aug 10, 2023
82f66ec
handle missing patch cases
jNullj Aug 10, 2023
438a1e6
fix missing fetch in node16
jNullj Aug 11, 2023
8677c52
fix typo
jNullj Aug 11, 2023
6354ba2
fix typo
jNullj Aug 11, 2023
d16d987
changed from diff parse to json usage
jNullj Aug 11, 2023
7f9f94e
fix keys issues
jNullj Aug 11, 2023
2a0573a
fix parent dependency version check
jNullj Aug 11, 2023
a1c24dd
Merge branch 'badges:master' into feat/9287/docusaurus-swizzled-warni…
jNullj Aug 11, 2023
5ccbb69
Merge branch 'master' into feat/9287/docusaurus-swizzled-warning-ci
jNullj Aug 11, 2023
372d18f
Apply suggestions from code review - Improve description and text format
jNullj Aug 13, 2023
c4a41f6
Fix comment table format
jNullj Aug 13, 2023
11fbd0b
Merge branch 'master' into feat/9287/docusaurus-swizzled-warning-ci
jNullj Aug 13, 2023
da3cf2e
fix type
jNullj Aug 13, 2023
c77af69
Add docusaurus-swizzled-warning to dependabot
jNullj Aug 13, 2023
db2e03c
refactor: remove node-fetch and use octokit insted
jNullj Aug 13, 2023
81f020f
improve code readability
jNullj Aug 13, 2023
9e1c2e4
fix type
jNullj Aug 13, 2023
637d122
Merge branch 'master' into feat/9287/docusaurus-swizzled-warning-ci
jNullj Aug 14, 2023
640567d
change pull_request_target to pull_request
jNullj Aug 18, 2023
78551ff
Improve action log
jNullj Aug 18, 2023
5311d03
Log old and new version
jNullj Aug 18, 2023
6f867f7
Merge branch 'master' into feat/9287/docusaurus-swizzled-warning-ci
jNullj Aug 18, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .github/actions/docusaurus-swizzled-warning/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
name: 'Docusaurus swizzled component changes warning'
jNullj marked this conversation as resolved.
Show resolved Hide resolved
description: 'Check for changes in Docusaurus components which are swizzled and prints out a warning'
jNullj marked this conversation as resolved.
Show resolved Hide resolved
branding:
icon: 'alert-triangle'
color: 'yellow'
inputs:
github-token:
description: 'The GITHUB_TOKEN secret'
required: true
runs:
using: 'node16'
main: 'index.js'
75 changes: 75 additions & 0 deletions .github/actions/docusaurus-swizzled-warning/helpers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
'use strict'

/**
* Returns info about all files changed in a PR (max 3000 results)
*
* @param {object} client hydrated octokit ready to use for GitHub Actions
* @param {string} owner repo owner
* @param {string} repo repo name
* @param {number} pullNumber pull request number
* @returns {object[]} array of object that describe pr changed files - see https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests-files
*/
async function getAllFilesForPullRequest(client, owner, repo, pullNumber) {
const perPage = 100 // Max number of items per page
let page = 1 // Start with the first page
let allFiles = []
while (true) {
const response = await client.rest.pulls.listFiles({
owner,
repo,
pull_number: pullNumber,
per_page: perPage,
page,
})

if (response.data.length === 0) {
// Break the loop if no more results
break
}

allFiles = allFiles.concat(response.data)
page++ // Move to the next page
}
return allFiles
}

/**
* Get a list of files changed betwen two tags for a github repo
*
* @param {object} client hydrated octokit ready to use for GitHub Actions
* @param {string} owner repo owner
* @param {string} repo repo name
* @param {string} baseTag base tag
* @param {string} headTag head tag
* @returns {string[]} Array listing all changed files betwen the base tag and the head tag
*/
async function getChangedFilesBetweenTags(
client,
owner,
repo,
baseTag,
headTag,
) {
const response = await client.rest.repos.compareCommits({
owner,
repo,
base: baseTag,
head: headTag,
})

return response.data.files.map(file => file.filename)
}

function findKeyEndingWith(obj, ending) {
for (const key in obj) {
if (key.endsWith(ending)) {
return key
}
}
}

module.exports = {
getAllFilesForPullRequest,
getChangedFilesBetweenTags,
findKeyEndingWith,
}
135 changes: 135 additions & 0 deletions .github/actions/docusaurus-swizzled-warning/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
'use strict'

const core = require('@actions/core')
const github = require('@actions/github')
const fetch = require('node-fetch')
const {
getAllFilesForPullRequest,
getChangedFilesBetweenTags,
findKeyEndingWith,
} = require('./helpers')

async function run() {
try {
const token = core.getInput('github-token', { required: true })

const { pull_request: pr } = github.context.payload
if (!pr) {
throw new Error('Event payload missing `pull_request`')
}

const client = github.getOctokit(token)
const packageName = 'docusaurus-theme-openapi'
const packageParentName = 'docusaurus-preset-openapi'
const overideComponents = ['Curl', 'Response']
const messageTemplate = `<table><thead><tr><th>
Copy link
Member

Choose a reason for hiding this comment

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

Minor points, but:

  1. If we have a look at your screenshot, we can see the table looks wierd because not all of the table rows have the same number of columns. Making these headers will fix that.

Screenshot at 2023-08-12 19-19-02

  1. We're using <th> for every cell in this table, but <th> is only for headers. The "normal" cells should be <td>s.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updating this, not sure what you mean at 1, but i used colspan="2" to fix the visual issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After testing
image

⚠️ This PR contains changes to components of ${packageName} we've overridden
</th></tr></thead>
<tbody><tr><th>
We need to watch out for changes to the ${overideComponents.join(
', ',
)}components
jNullj marked this conversation as resolved.
Show resolved Hide resolved
</th></tr>
`

if (
['dependabot[bot]', 'dependabot-preview[bot]'].includes(pr.user.login)
) {
const files = await getAllFilesForPullRequest(
client,
github.context.repo.owner,
github.context.repo.repo,
pr.number,
)

for (const file of files) {
if (file.filename !== 'package-lock.json') {
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

In general, it is harder to read large blocks of code that are deeply nested.

We can often work round nesting by taking some code like

if (condition) {
  // do stuff
}

and re-writing it as

if (!condition) {
  return
}

// do stuff

I think we could make this whole run function easier to read by getting rid of a couple of layers of nesting here. For example:

if (!['dependabot[bot]', 'dependabot-preview[bot]'].includes(pr.user.login)) {
  return
}

const file = files.filter(f => f.filename === 'package-lock.json')[0]
if (file === undefined) {
   return
}

// do something with file

This would also make it clearer that we are not expecting to process more than one package-lock.json.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I improved code readability with 81f020f
then fixed a type with 9e1c2e4


const pkgLockNewJson = await (await fetch(file.raw_url)).json()
const pkgLockOldJson = await (
await fetch(
`https://raw.githubusercontent.com/${github.context.repo.owner}/${github.context.repo.repo}/master/${file.filename}`,
)
).json()
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to pull in node-fetch as a dependency and request these URLs. We've got an OctoKit instance in scope, so I think we should be able to use:

octokit.rest.repos.getContent({
  owner,
  repo,
  path,
  ref?,
});

to replace both these calls.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am new to octokit, I will look into this and add this if applicable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

solved with db2e03c
notice that files are > 1MB therefor an getContent won't return content (tried even with raw)
I had to use the sha and make a new blob request.

const oldVesionModuleKey = findKeyEndingWith(
pkgLockOldJson.packages,
`node_modules/${packageName}`,
)
const newVesionModuleKey = findKeyEndingWith(
pkgLockNewJson.packages,
`node_modules/${packageName}`,
)
let oldVersion = pkgLockOldJson.packages[oldVesionModuleKey].version
let newVersion = pkgLockNewJson.packages[newVesionModuleKey].version

const oldVesionModuleKeyParent = findKeyEndingWith(
pkgLockOldJson.packages,
`node_modules/${packageParentName}`,
)
const newVesionModuleKeyParent = findKeyEndingWith(
pkgLockNewJson.packages,
`node_modules/${packageParentName}`,
)
const oldVersionParent =
pkgLockOldJson.packages[oldVesionModuleKeyParent].dependencies[
packageName
].substring(1)
const newVersionParent =
pkgLockNewJson.packages[newVesionModuleKeyParent].dependencies[
packageName
].substring(1)

// if parent dependency is higher version then existing
// npm install will retrive the newer version from the parent dependency
if (oldVersionParent > oldVersion) {
oldVersion = oldVersionParent
}
if (newVersionParent > newVersion) {
newVersion = newVersionParent
}

Copy link
Member

Choose a reason for hiding this comment

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

It would be useful here if we could write a log message here (again, core.info()) with oldVersion and newVersion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added with 5311d03

if (newVersion !== oldVersion) {
const pkgChangedFiles = await getChangedFilesBetweenTags(
client,
'cloud-annotations',
'docusaurus-openapi',
`v${oldVersion}`,
`v${newVersion}`,
)
const changedComponents = overideComponents.filter(
componenet =>
pkgChangedFiles.filter(
path =>
path.includes('docusaurus-theme-openapi/src/theme') &&
path.includes(componenet),
).length > 0,
)
const versionReport = `<tr><th> Old version </th><th> ${oldVersion} </th></tr>
<tr><th> New version </th><th> ${newVersion} </th></tr>
`
const changedComponentsReport = `<tr><th> Overide components changed </th><th> ${changedComponents.join(
', ',
)} </th></tr></tbody></table>
`
const body = messageTemplate + versionReport + changedComponentsReport
await client.rest.issues.createComment({
owner: github.context.repo.owner,
repo: github.context.repo.repo,
issue_number: pr.number,
body,
})

core.debug('Found changes and posted comment, done.')
return
}
}
core.debug('No changes found, done.')
}
} catch (error) {
core.setFailed(error.message)
}
}

run()
Loading
Loading