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

Conversation

jNullj
Copy link
Collaborator

@jNullj jNullj commented Aug 11, 2023

Add new action and workflow that runs on new dependabot PR.
The action checks changes in components of docusaurus-theme-openapi that are swizzled and warns maintainers to review those changes.
For more info see #9287

image

Solves #9287

@socket-security
Copy link

socket-security bot commented Aug 11, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@actions/github 5.1.1 environment +3 316 kB thboop
@actions/core 1.10.0 filesystem, environment +2 211 kB thboop

@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2023

Messages
📖 ✨ Thanks for your contribution to Shields, @jNullj!

Generated by 🚫 dangerJS against 6f867f7

@jNullj jNullj marked this pull request as ready for review August 11, 2023 14:59
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. This looks like it will be really useful.

I've had a read over the code and left you a first pass of comments. I've not actually run the code yet though. JS actions are very powerful, but one of the big issues is that it is quite hard to just check code like this out and run it locally to make sure it works.

How have you been testing this? Did you do it all in a test repo, or were you able to run bits of the code locally?

.github/actions/docusaurus-swizzled-warning/action.yml Outdated Show resolved Hide resolved
.github/actions/docusaurus-swizzled-warning/action.yml Outdated Show resolved Hide resolved
.github/actions/docusaurus-swizzled-warning/index.js Outdated Show resolved Hide resolved
Comment on lines 50 to 55
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.

Comment on lines 35 to 48
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 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

Comment on lines 12 to 14
"dependencies": {
"@actions/core": "^1.10.0",
"@actions/github": "^5.1.1",
Copy link
Member

@chris48s chris48s Aug 12, 2023

Choose a reason for hiding this comment

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

If we are adding a package.json with dependencies here, we should register it with dependabot, like this:

# close-bot package dependencies
- package-ecosystem: npm
directory: '/.github/actions/close-bot'
schedule:
interval: weekly
day: friday
time: '12:00'
open-pull-requests-limit: 99
rebase-strategy: disabled

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 c77af69

@chris48s chris48s added the developer-experience Dev tooling, test framework, and CI label Aug 12, 2023
Co-authored-by: chris48s <chris48s@users.noreply.github.com>
@jNullj
Copy link
Collaborator Author

jNullj commented Aug 13, 2023

Thanks for working on this. This looks like it will be really useful.

I've had a read over the code and left you a first pass of comments. I've not actually run the code yet though. JS actions are very powerful, but one of the big issues is that it is quite hard to just check code like this out and run it locally to make sure it works.

How have you been testing this? Did you do it all in a test repo, or were you able to run bits of the code locally?

Thank you for the code review and the useful insights.

Regarding testing - I used a forked repo on github for testing, I think I might be able to run it locally if i look for some testing tools or try to simulate github with a crafted payload.

@jNullj jNullj requested a review from chris48s August 13, 2023 17:22
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

OK. Latest changes look great - thanks. I've done a bit of testing on this in another repo. I found it a bit tricky to test this, but my experience is that when it comes to these GitHub actions, there is a large extent to which we can "set it and forget it". We don't tend to be constantly changing them. So I'm not too worried about this being a bit of a pain to test.

I've left a few final comments inline, but this is 99% done.

Comment on lines 136 to 140
core.debug('Found changes and posted comment, done.')
return
}

core.debug('No changes found, done.')
Copy link
Member

Choose a reason for hiding this comment

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

Can we make these log statements use core.info() instead of core.debug() so they always get logged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. This would be easier to understand.
Added with 78551ff

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

@@ -0,0 +1,22 @@
name: Docusaurus swizzled component changes warning
on:
pull_request_target:
Copy link
Member

Choose a reason for hiding this comment

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

This workflow doesn't actually need to run on pull_request_target. It can just run on pull_request. I'm aware this is a copy & paste from close-bot, which probably could have also just used pull_request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yea, my bad, didn't notice this when I started with close-bot as a ref for the file.
Fixed with 640567d

Change core.debug to core.info so it always shows and not only when in debug.
Log old and new versions for the Docusaurus swizzled component changes warning workflow.
@jNullj jNullj requested a review from chris48s August 18, 2023 20:51
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

Sweeeet. Thanks for working on this one

@chris48s chris48s added this pull request to the merge queue Aug 18, 2023
Merged via the queue into badges:master with commit 7d44ebf Aug 18, 2023
19 checks passed
@calebcartwright
Copy link
Member

Nicely done @jNullj!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants