feat: add preview command to covector #187

minkimcello · 2021-04-08T22:56:53Z

Motivation

To continue the work of #179 and implement the actual preview command within covector. (179 set up the preview workflow and the basic conditionals within the action and covector to allow preview commands).

Approach

Made PR label configurable. If none is provided in the workflow it will use preview as default. You can specify your own like this:
```
- name: covector preview
  uses: jbolda/covector/packages/action@mk/run-preview
  id: covector
  with:
    command: preview
    label: whateverLabel
```
- If the preview workflow runs and the required label is not applied to the PR, it won't publish any preview packages.
Also made version template configurable. At the moment there are only two options: date and sha. If none is provided, the default will be date:
```
- name: covector preview
  uses: jbolda/covector/packages/action@mk/run-preview
  id: covector
  with:
    command: preview
    label: whateverLabel
    previewVersion: sha
```
- The templates look like this:
  - date = 1.0.0-${Date.now()} = 1.0.0-1618444347550
  - sha = 1.0.0-${commit.substring(0, 7)} = 1.0.0-abcdefg
Cleaned up commented out code from feat: preparations of covector/action for building and publishing preview packages #179
apply has a new argument that determines if a dependency bump should use its normal semver.inc(bumpType) or semver.valid(previewVersionTemplate)
- But it looks like bumpDep is never triggered? See TODOs below.
preview command inside covector pretty much runs version and then publish but without deleting the change files or modifying the changelogs.
- I also excluded postPublish command
Added preview label to the version PR creating step inside run-version-or-publish workflow to automatically opt-in to publishing preview packages. This takes care of "case 3"

TODOs

Generate comment on PR after publishing previews. ~~Will do this in a separate PR.~~ feat: pr comment generator for covector preview #191
~~I set the timeout to x2 the regular length because we're doing twice the work in preview. Should I keep it that way or just set it back to normal?~~ No.
~~packagesPublished set here seems to always return an empty string on account of covectored[pkg].published boolean never being set.~~ Separate PR
I wanted to see bumpDep in action so I ran the preview action with only change files for @covector/files hoping it'll update the @covector/files dependencies for apply, covector, assemble, and changelog but it only bumped covector, files, and action Fixed in bug: Fix bumping error #198:

Screenshot

How does env.NODE_AUTH_TOKEN get used? I can't find it in the action. I haven't been publishing any packages with covector yet but I'm guessing when it actually tries to publish it won't be authenticated yet.
I haven't done npm tags yet. Should I include that in this PR? Doesn't need to be in the same PR because pre-release semver won't be tagged as latest (1.0.0 will still be latest if you publish 1.0.1-12345) Separate PR

cowboyd · 2021-04-15T10:57:04Z

How does this relate to when you have a preview, but no PR?

In other words, the mainline is v1, but we're working on v2, so everything on v2 should get released to a preview?

minkimcello · 2021-04-15T12:19:14Z

How does this relate to when you have a preview, but no PR?

In other words, the mainline is v1, but we're working on v2, so everything on v2 should get released to a preview?

Jacob is working on that part.

jbolda · 2021-04-16T15:21:44Z

Yep, prereleases coming in #180.

jbolda · 2021-04-19T20:22:07Z

packagesPublished set here seems to always return an empty string on account of covectored[pkg].published boolean never being set.

Seems like an oversight. We can probably open an issue and fix it in a separate PR.

I wanted to see bumpDep in action...

I believe the caveat here is that it will only bump the dependencies if the main package version also has a bump. Do you think that was the case? It is kind of hard to tell from that output.

How does env.NODE_AUTH_TOKEN get used? I can't find it in the action. I haven't been publishing any packages with covector yet but I'm guessing when it actually tries to publish it won't be authenticated yet.

The action/setup-node makes use of it. It sets a reference to that env var in the .nmprc if I recall correctly.

I haven't done npm tags yet. Should I include that in this PR? Doesn't need to be in the same PR because pre-release semver won't be tagged as latest (1.0.0 will still be latest if you publish 1.0.1-12345)

I am going to need this for prereleases as well. I think we can address this in a follow up PR.

packages/action/src/index.ts

packages/covector/src/run.ts

packages/apply/index.ts

packages/covector/src/run.ts

jbolda · 2021-04-20T20:13:20Z

I wanted to see bumpDep in action...

I believe the caveat here is that it will only bump the dependencies if the main package version also has a bump. Do you think that was the case? It is kind of hard to tell from that output.

So say there's a change file to bump package-A (and none for package-B) but package-A is listed as a dependency in package-B, you're saying this is how it would look?:
{
  "name": "package-A",
  "version": "1.0.0-abcdef"
},
{
  "name": "package-B",
  "version": "1.0.0",
  "dependencies:": {
    "package-A": "1.0.0-abcdef"
  }
}

I believe it would look like:

{
  "name": "package-A",
  "version": "1.0.0-abcdef"
},
{
  "name": "package-B",
  "version": "1.0.0-abcdef",
  "dependencies:": {
    "package-A": "1.0.0-abcdef"
  }
}

The bump to package-A also creates a bump for package-B with a patch (or it will use whatever higher bump if you are already directly bumping package-B otherwise).

How does env.NODE_AUTH_TOKEN get used? I can't find it in the action. I haven't been publishing any packages with covector yet but I'm guessing when it actually tries to publish it won't be authenticated yet.

The action/setup-node makes use of it. It sets a reference to that env var in the .nmprc if I recall correctly.

I mean it gets passed into the action here but I can't find the instance of the action utilizing the variable.

The action/setup-node sets it in a config file here which npm publish then picks up on when it runs. If you were running it locally, you would like npm login and it would set that same token in a file that it references locally when publishing. If you check your HOME DIR for an .npmrc, it will contain something like this:

registry.npmjs.org/:_authToken={token-here}

So locally it gets hard-coded, but in CI, it relies on an env var so you can pass it in through your secrets.

minkimcello · 2021-04-20T20:51:56Z

I wanted to see bumpDep in action...

I believe the caveat here is that it will only bump the dependencies if the main package version also has a bump. Do you think that was the case? It is kind of hard to tell from that output.

So say there's a change file to bump package-A (and none for package-B) but package-A is listed as a dependency in package-B, you're saying this is how it would look?:
{
  "name": "package-A",
  "version": "1.0.0-abcdef"
},
{
  "name": "package-B",
  "version": "1.0.0",
  "dependencies:": {
    "package-A": "1.0.0-abcdef"
  }
}
I believe it would look like:
{
  "name": "package-A",
  "version": "1.0.0-abcdef"
},
{
  "name": "package-B",
  "version": "1.0.0-abcdef",
  "dependencies:": {
    "package-A": "1.0.0-abcdef"
  }
}
The bump to package-A also creates a bump for package-B with a patch (or it will use whatever higher bump if you are already directly bumping package-B otherwise).

So if I have a change file for @covector/files, it should've applied bumps to @covector/apply, covector, @covector/assemble, and @covector/changelog but as you can see in the screenshot, it only applied bumps to @covector/files, covector, and @covector/action.

How does env.NODE_AUTH_TOKEN get used? I can't find it in the action. I haven't been publishing any packages with covector yet but I'm guessing when it actually tries to publish it won't be authenticated yet.

The action/setup-node makes use of it. It sets a reference to that env var in the .nmprc if I recall correctly.

I mean it gets passed into the action here but I can't find the instance of the action utilizing the variable.

The action/setup-node sets it in a config file here which npm publish then picks up on when it runs. If you were running it locally, you would like npm login and it would set that same token in a file that it references locally when publishing. If you check your HOME DIR for an .npmrc, it will contain something like this:
registry.npmjs.org/:_authToken={token-here}
So locally it gets hard-coded, but in CI, it relies on an env var so you can pass it in through your secrets.

So the setup-node action sets the config file and so for the rest of the workflow it would have access to config but why does the token need to be passed into covector's action?

- uses: actions/setup-node@v2
  with:
    registry-url: "https://registry.npmjs.org"
- name: git config
  run: |
    git config --global user.name "${{ github.event.pusher.name }}"
    git config --global user.email "${{ github.event.pusher.email }}"
- run: npm ci --ignore-scripts
- run: npm run build
- run: npm run pkg -w action
- name: covector version-or-publish
  uses: ./packages/action
  id: covector


  env:
    NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}


  with:
    token: ${{ secrets.GITHUB_TOKEN }}
    command: "version-or-publish"
    createRelease: true

jbolda · 2021-04-27T18:50:17Z

So if I have a change file for @covector/files, it should've applied bumps to @covector/apply, covector, @covector/assemble, and @covector/changelog but as you can see in the screenshot, it only applied bumps to @covector/files, covector, and @covector/action.

Hmm, to get the whole and properly bumping, we chain a few functions together. The only things I can guess are bug or we are missing one of those links in the chain?

So the setup-node action sets the config file and so for the rest of the workflow it would have access to config but why does the token need to be passed into covector's action?

- uses: actions/setup-node@v2
  with:
    registry-url: "https://registry.npmjs.org"
- name: git config
  run: |
    git config --global user.name "${{ github.event.pusher.name }}"
    git config --global user.email "${{ github.event.pusher.email }}"
- run: npm ci --ignore-scripts
- run: npm run build
- run: npm run pkg -w action
- name: covector version-or-publish
  uses: ./packages/action
  id: covector


  env:
    NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}


  with:
    token: ${{ secrets.GITHUB_TOKEN }}
    command: "version-or-publish"
    createRelease: true

It sets up the .npmrc with a value that references that env var instead of hard coding it.

.changes/preview-action-bump.md

jbolda · 2021-04-28T13:57:54Z

packages/apply/index.ts

@@ -300,12 +309,12 @@ const bumpMain = ({
    if (pkg.vfile.extname === ".json") {
      // for javascript
      // @ts-ignore TODO bumpType should be narrowed to meet ReleaseType
-      let version = semver.inc(pkg.pkg.version, bumpType);
+      let version = previewVersion ? semver.valid(`${pkg.pkg.version}-${previewVersion}`) : semver.inc(pkg.pkg.version, bumpType);


Can we add a couple tests in apply to confirm that version number that is output (and see we don't break it in the future)?

https://github.com/jbolda/covector/pull/187/files#diff-616402c1e04e5b05910c65ff8041f2fe6f7c7491efa1e268b6a4239de93413dfR1093-R1232

packages/covector/src/run.ts

jbolda · 2021-04-29T18:38:04Z

packages/apply/index.test.ts

+        "}\n"
+    );
+
+    expect({


Looking at the snapshot, consoleDir and consoleLog don't have any data, Is that expected? I presume there would be some output here.

The reason why there's no console log is because it's been omitted but I just added it here

packages/apply/index.test.ts

jbolda · 2021-04-29T18:55:54Z

packages/covector/__snapshots__/index.test.ts.snap

+            },
+            "version": "0.6.2-branch-name.12345",
+          },
+          "version": "0.6.2-branch-name.12345",


Looks like we are getting the version here 👍 . It doesn't seem like it is attempting to publish it though, I imagine because of the change files? May be worth creating a new fixture if it is painful finding one that works for your tests.

It doesn't seem like it is attempting to publish it though

Doesn't this mean it did try to publish?

I was looking for something like "Checking if [email protected] is already published with: echo 0.5.2",, but not seeing it which is kind of confusing. Like I don't see any of the output mention trying to publish tauri.js.

But yea, you are right, it does say "tauri.js [publish]: echo publishing tauri.js would happen here",. I wonder if it is worth checking the version there by adjusting that publish command to echo it out?

So in that fixture, there's a change file for tauri and tauri-updater. tauri-updater isn't in the snapshot because publish is set to false. tauri.js gets a preview bump because it lists tauri as a dependency. But that doesn't explain why tauri isn't getting bumped and why tauri.js isn't trying to publish. This might have to do with the bug I pointed out about the interdependent bumping logic.

packages/covector/__snapshots__/index.test.ts.snap

minkimcello · 2021-05-04T11:39:54Z

packages/covector/__snapshots__/index.test.ts.snap

+    Array [
+      "bumping package-b to 0.0.1-branch-name.12345 to publish a preview",
+    ],
+    Array [
+      "bumping package-a to 0.0.1-branch-name.12345 to publish a preview",
+    ],
+    Array [
+      "package-a [publish]: echo publishing package-a would happen here",
+    ],
+    Array [
+      "publishing package-a would happen here",
+    ],
+    Array [
+      "package-b [publish]: echo publishing package-b would happen here",
+    ],
+    Array [
+      "publishing package-b would happen here",
+    ],


In this new fixture I have package-a and package-b and package-a has package-b as one of its dependencies. I added a changefile only for package-b to see if it'll bump the dependency inside package-a.

So maybe the issue we saw earlier comes from when different managers are mixed in together?

So maybe the issue we saw earlier comes from when different managers are mixed in together?

That would be surprising to me. It might be worth adding a package-c with the same manager as your change to confirm?

should package-c have any dependencies? or have it be added as a dependency to package-a or package-b?

minkimcello · 2021-05-04T11:40:27Z

packages/covector/__snapshots__/index.test.ts.snap

+          "pkg": Object {
+            "dependencies": Object {
+              "package-b": "0.0.1-branch-name.12345",
+            },
+            "name": "package-a",
+            "version": "0.0.1-branch-name.12345",
+          },


* Fix overlooked refactor of a variable in apply * Rename package files in config to match the packages json files * Reintroduce accidentally deleted toml file in fixtures * Fix change files with the correct package names

jbolda

🙌

minkimcello added the preview label Apr 8, 2021

minkimcello force-pushed the mk/run-preview branch from 9e71839 to 9afbd3e Compare April 9, 2021 15:33

minkimcello mentioned this pull request Apr 9, 2021

feat: preparations of covector/action for building and publishing preview packages #179

Merged

4 tasks

minkimcello added enhancement New feature or request and removed enhancement New feature or request labels Apr 9, 2021

minkimcello force-pushed the mk/run-preview branch 2 times, most recently from 2e9d04a to b6366fc Compare April 13, 2021 21:21

Base automatically changed from mk/previews to main April 13, 2021 23:06

minkimcello force-pushed the mk/run-preview branch 5 times, most recently from e6a75ef to b851087 Compare April 14, 2021 18:44

minkimcello removed the preview label Apr 14, 2021

minkimcello force-pushed the mk/run-preview branch from c92e409 to b69ebc2 Compare April 14, 2021 22:18

minkimcello changed the title ~~[WIP] feat: add preview command to covector~~ feat: add preview command to covector Apr 15, 2021

minkimcello marked this pull request as ready for review April 15, 2021 01:58

minkimcello mentioned this pull request Apr 15, 2021

fix: update preview workflow #193

Merged

minkimcello mentioned this pull request Apr 16, 2021

feat: pr comment generator for covector preview #191

Merged

4 tasks

jbolda mentioned this pull request Apr 17, 2021

config validation command #37

Open

minkimcello requested a review from jbolda April 19, 2021 14:00

jbolda reviewed Apr 19, 2021

View reviewed changes

minkimcello added 4 commits April 20, 2021 14:07

Disable workflows and update location of preview action

42901a7

Specify label and previewVersion inputs for action manifest

63bb96e

Add conditional for version bumping for previews

540aa65

Add versioning steps for preview command

e7fcba9

minkimcello force-pushed the mk/run-preview branch from d48e48f to d559317 Compare April 27, 2021 21:19

Add action argument for branch identifier

670048b

minkimcello force-pushed the mk/run-preview branch from d559317 to 670048b Compare April 27, 2021 21:22

minkimcello requested a review from jbolda April 27, 2021 21:27

jbolda reviewed Apr 28, 2021

View reviewed changes

minkimcello added 6 commits April 28, 2021 11:21

Change change files to minor bump

4eb4a25

Remove unnecessary argument

4ce8de0

Fix apply bug for bumpDep

8d30081

Add test for preview apply

466a77f

Add test for preview covector

af9490e

Remove obsolete snapshots

9a68d1f

jbolda reviewed Apr 29, 2021

View reviewed changes

packages/apply/index.test.ts Show resolved Hide resolved

jbolda reviewed Apr 29, 2021

View reviewed changes

packages/covector/__snapshots__/index.test.ts.snap Outdated Show resolved Hide resolved

minkimcello added 3 commits April 30, 2021 13:27

Add console log for preview version bumps

7f11f9e

Update snapshot

b24a650

New fixture for preview

1423cd0

minkimcello commented May 4, 2021

View reviewed changes

minkimcello added 3 commits May 4, 2021 12:41

New new fixture

838380e

Fix rust bug

e7cd5ce

Make test use new fixture

ecb1ea6

minkimcello mentioned this pull request May 5, 2021

bug: Fix bumping error #198

Merged

bug: Fix bumping error (#198)

a0a3f5f

* Fix overlooked refactor of a variable in apply * Rename package files in config to match the packages json files * Reintroduce accidentally deleted toml file in fixtures * Fix change files with the correct package names

jbolda approved these changes May 5, 2021

View reviewed changes

minkimcello merged commit 5506b19 into main May 5, 2021

minkimcello deleted the mk/run-preview branch May 5, 2021 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add preview command to covector #187

feat: add preview command to covector #187

minkimcello commented Apr 8, 2021 •

edited

Loading

cowboyd commented Apr 15, 2021

minkimcello commented Apr 15, 2021

jbolda commented Apr 16, 2021

jbolda commented Apr 19, 2021

jbolda commented Apr 20, 2021 •

edited

Loading

minkimcello commented Apr 20, 2021

jbolda commented Apr 27, 2021

jbolda Apr 28, 2021

minkimcello Apr 29, 2021

jbolda Apr 29, 2021

minkimcello Apr 30, 2021

jbolda Apr 29, 2021

minkimcello Apr 30, 2021

jbolda Apr 30, 2021

jbolda Apr 30, 2021

minkimcello Apr 30, 2021

minkimcello May 4, 2021

minkimcello May 4, 2021

jbolda May 4, 2021

minkimcello May 4, 2021

minkimcello May 4, 2021

jbolda left a comment

feat: add preview command to covector #187

feat: add preview command to covector #187

Conversation

minkimcello commented Apr 8, 2021 • edited Loading

Motivation

Approach

TODOs

cowboyd commented Apr 15, 2021

minkimcello commented Apr 15, 2021

jbolda commented Apr 16, 2021

jbolda commented Apr 19, 2021

jbolda commented Apr 20, 2021 • edited Loading

minkimcello commented Apr 20, 2021

jbolda commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbolda left a comment

Choose a reason for hiding this comment

minkimcello commented Apr 8, 2021 •

edited

Loading

jbolda commented Apr 20, 2021 •

edited

Loading