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

api-diff tool: pick an omicron PR, see diff for generated API #1819

Merged
merged 6 commits into from
Nov 30, 2023

Conversation

david-crespo
Copy link
Collaborator

@david-crespo david-crespo commented Nov 26, 2023

YES

$ ./tools/deno/api-diff.ts -h
Display changes to API client caused by a given Omicron PR. Works by downloading
the OpenAPI spec before and after, generating clients in temp dirs, and diffing.

Requirements:
  - Deno (which you have if you're seeing this message)
  - GitHub CLI (gh)

Usage:
  ./tools/deno/api-diff.ts [-f] [PR number]
  ./tools/deno/api-diff.ts -h

Flags:
  -f, --force          Download spec and regen client even if dir already exists
  -h, --help           Show this help message

Parameters:
  PR number <int>: If left out, interactive picker is shown

Could add:

  • do this for a single commit on main
  • take a PR number as an arg and skip the picker UI (would allow us to see diff for closed PRs)
  • take two arbitrary commits as base and head
2023-11-25-api-diff-2.mp4

Copy link

vercel bot commented Nov 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Nov 30, 2023 10:01pm

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 453 files.

Valid Invalid Ignored Fixed
402 1 50 0
Click to see the invalid file list
  • tools/deno/api-diff.ts

@@ -0,0 +1,75 @@
#! /usr/bin/env -S deno run --allow-run --allow-net --allow-read --allow-write --allow-env

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

@david-crespo
Copy link
Collaborator Author

david-crespo commented Nov 27, 2023

I realized it would be shorter in bash because we're not really doing anything but calling a bunch of commands, so I tried it in bash. Then anything in bash would be better in nushell (see, for example, how trivial it was to add the prNum positional arg), so I tried it in nushell. I do like nu scripts a lot: they're the best of both worlds in terms of a natural API for calling shell commands and working with the output plus modern constructs for looping and conditionals and whatever else. See, for example:

let pr = (gh api graphql -f $query | from json | get data.repository.pullRequest)

Since it would be very few of us using these scripts and all they require is having nu on your path, I am wondering if it might be worth it to actually use nu for these things. Recall I did the same exercise a little while ago for the bump-omicron script and it came out nice too: #1487

@zephraph
Copy link
Contributor

Since it would be very few of us using these scripts and all they require is having nu on your path, I am wondering if it might be worth it to actually use nu for these things.

As interesting as our excursions with nushell were and as terse as the output can be, I'm not sure it's really worth it to introduce it as another dependency in the toolchain just to save a few lines.

A few points:

  • Nushell is a bit cavalier about their updates and I ran into frequent breaking updates between versions. Until they hit 1.0 and we have a higher guarantee of safety I'd be a little hesitant to build tools on it that we anticipate the broader team using.
  • While it's simpler on the face of it, it's still another thing to learn. It would be nice to reduce the overall surface area of tooling.

My preference would be to roll with the deno or bash scripts just given that they're a known quantity.

@david-crespo
Copy link
Collaborator Author

Cool, I'll stick with deno then.

@david-crespo david-crespo merged commit 1c790d2 into main Nov 30, 2023
8 checks passed
@david-crespo david-crespo deleted the api-diff branch November 30, 2023 22:08
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Dec 7, 2023
oxidecomputer/console@ae8218d...1802c28

* [1802c285](oxidecomputer/console@1802c285) oxidecomputer/console#1839
* [ce09b547](oxidecomputer/console@ce09b547) bump postcss-pseudo-classes for fake vuln
* [e09b803b](oxidecomputer/console@e09b803b) might as well get vitest 1.0 in there too
* [83dd73ee](oxidecomputer/console@83dd73ee) minor bumps for react router, msw, vite, tailwind, recharts
* [6ae6beeb](oxidecomputer/console@6ae6beeb) oxidecomputer/console#1829
* [a0bf47aa](oxidecomputer/console@a0bf47aa) oxidecomputer/console#1836
* [6c9420ad](oxidecomputer/console@6c9420ad) oxidecomputer/console#1835
* [64e97b01](oxidecomputer/console@64e97b01) api-diff also takes a commit
* [22bef0bb](oxidecomputer/console@22bef0bb) oxidecomputer/console#1833
* [2fe50f51](oxidecomputer/console@2fe50f51) oxidecomputer/console#1810
* [faadb6d3](oxidecomputer/console@faadb6d3) oxidecomputer/console#1832
* [9e82f9ab](oxidecomputer/console@9e82f9ab) oxidecomputer/console#1811
* [5e11fd83](oxidecomputer/console@5e11fd83) tweak api-diff
* [dae20577](oxidecomputer/console@dae20577) oxidecomputer/console#1827
* [ed0ef62e](oxidecomputer/console@ed0ef62e) minor tweaks to api-diff script
* [1c790d27](oxidecomputer/console@1c790d27) oxidecomputer/console#1819
* [97be7724](oxidecomputer/console@97be7724) oxidecomputer/console#1826
* [87f4d8b8](oxidecomputer/console@87f4d8b8) oxidecomputer/console#1814
* [65ae1212](oxidecomputer/console@65ae1212) oxidecomputer/console#1820
* [5a6dcea7](oxidecomputer/console@5a6dcea7) oxidecomputer/console#1822
* [4e1bbe13](oxidecomputer/console@4e1bbe13) oxidecomputer/console#1821
* [17408f64](oxidecomputer/console@17408f64) oxidecomputer/console#1813
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Dec 7, 2023
### User-facing changes

* [1802c285](oxidecomputer/console@1802c285)
oxidecomputer/console#1839
* [6ae6beeb](oxidecomputer/console@6ae6beeb)
oxidecomputer/console#1829
* [a0bf47aa](oxidecomputer/console@a0bf47aa)
oxidecomputer/console#1836
* [9e82f9ab](oxidecomputer/console@9e82f9ab)
oxidecomputer/console#1811
* [5a6dcea7](oxidecomputer/console@5a6dcea7)
oxidecomputer/console#1822

### All changes

oxidecomputer/console@ae8218d...1802c28

* [1802c285](oxidecomputer/console@1802c285)
oxidecomputer/console#1839
* [ce09b547](oxidecomputer/console@ce09b547)
bump postcss-pseudo-classes for fake vuln
* [e09b803b](oxidecomputer/console@e09b803b)
might as well get vitest 1.0 in there too
* [83dd73ee](oxidecomputer/console@83dd73ee)
minor bumps for react router, msw, vite, tailwind, recharts
* [6ae6beeb](oxidecomputer/console@6ae6beeb)
oxidecomputer/console#1829
* [a0bf47aa](oxidecomputer/console@a0bf47aa)
oxidecomputer/console#1836
* [6c9420ad](oxidecomputer/console@6c9420ad)
oxidecomputer/console#1835
* [64e97b01](oxidecomputer/console@64e97b01)
api-diff also takes a commit
* [22bef0bb](oxidecomputer/console@22bef0bb)
oxidecomputer/console#1833
* [2fe50f51](oxidecomputer/console@2fe50f51)
oxidecomputer/console#1810
* [faadb6d3](oxidecomputer/console@faadb6d3)
oxidecomputer/console#1832
* [9e82f9ab](oxidecomputer/console@9e82f9ab)
oxidecomputer/console#1811
* [5e11fd83](oxidecomputer/console@5e11fd83)
tweak api-diff
* [dae20577](oxidecomputer/console@dae20577)
oxidecomputer/console#1827
* [ed0ef62e](oxidecomputer/console@ed0ef62e)
minor tweaks to api-diff script
* [1c790d27](oxidecomputer/console@1c790d27)
oxidecomputer/console#1819
* [97be7724](oxidecomputer/console@97be7724)
oxidecomputer/console#1826
* [87f4d8b8](oxidecomputer/console@87f4d8b8)
oxidecomputer/console#1814
* [65ae1212](oxidecomputer/console@65ae1212)
oxidecomputer/console#1820
* [5a6dcea7](oxidecomputer/console@5a6dcea7)
oxidecomputer/console#1822
* [4e1bbe13](oxidecomputer/console@4e1bbe13)
oxidecomputer/console#1821
* [17408f64](oxidecomputer/console@17408f64)
oxidecomputer/console#1813
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