Publish types-registry to npm AND github package registry #661

sandersn · 2019-10-02T17:38:19Z

Publish types-registry to @definitelytyped/types-registry before publishing to types-registry.

Note that, for testing purposes, the time-limit per publish is 10 minutes instead of 1 week. I'll change that after this PR is merged and I have verified that it works.

You can see the results of my local testing; 0.1.456 was published from my machine using the below PR. 0.1.450 is a manual publish from @andrewbranch when we first tested the gihub package registry.

Plus cleanup needed to make it readable(ish).

Also 1. Rename "oldVersion" to "npmVersion" 2. Correctly log why we skipped publishing -- the predicate was backward.

sandersn · 2019-10-02T17:41:48Z

Hide whitespace helps a lot with this review since I moved a whole block of code into a function in order to call it twice.

sandersn · 2019-10-02T17:42:05Z

src/lib/npm-client.ts

@@ -9,11 +9,7 @@ import { createTgz } from "../util/tgz";
 import { identity, joinPaths, mapToRecord, recordToMap } from "../util/util";

 import { getSecret, Secret } from "./secrets";
-import { npmApi, npmRegistry, npmRegistryHostName } from "./settings";


sandersn · 2019-10-02T17:42:16Z

src/lib/npm-client.ts

@@ -9,11 +9,7 @@ import { createTgz } from "../util/tgz";
 import { identity, joinPaths, mapToRecord, recordToMap } from "../util/util";

 import { getSecret, Secret } from "./secrets";
-import { npmApi, npmRegistry, npmRegistryHostName } from "./settings";
-
-function packageUrl(packageName: string): string {


function was only used once so I inlined it

sandersn · 2019-10-02T17:42:42Z

src/lib/npm-client.ts

    }

-    private constructor(private readonly client: RegClient, private readonly auth: RegClient.Credentials) {}


readonly seems superfluous for a 20-line class.

sandersn · 2019-10-02T17:44:06Z

src/lib/npm-client.ts

-        return new this(new RegClient(config), { token });
+    static async create(config?: RegClient.Config, registryName: "github" | "npm" = "npm"): Promise<NpmPublishClient> {
+        if (registryName === "github") {
+            return new this(new RegClient(config), { token: await getSecret(Secret.GITHUB_PUBLISH_ACCESS_TOKEN) }, githubRegistry);


I added the secret github-publish-access-token on azure. It's using a token from my account for now, but after this merges we should give typescript-bot a publish token. Unfortunately, these tokens currently require full repo access, which I would prefer to not give to types-publisher.

sandersn · 2019-10-02T17:45:40Z

src/lib/npm-client.ts

+        return promisifyVoid(cb => {
+            if (dry) {
+                log(`(dry) Skip tag of ${packageName}@${distTag} as ${version}`);
+                cb(undefined);


Is there a less clunky way to write this? I haven't written much promisified code, so I fell back to my base knowledge of the monads+continuations. (It also made me want to rename cb to k or return_, but I refrained.)

I’m not familiar with promisifyVoid, but you could move if (dry) { ... } outside the closure and return Promise.resolve() instead of cb(undefined)?

It's a utility function lower in the file, but it doesn't seem that useful.

I'll use return Promise.resolve().

sandersn · 2019-10-02T17:46:27Z

src/lib/npm-client.ts

+                log(`(dry) Skip tag of ${packageName}@${distTag} as ${version}`);
+                cb(undefined);
+            } else {
+                this.client.distTags.add(this.registry, { package: packageName, version, distTag, auth: this.auth }, cb);


I switched from tag to distTags.add because it allows me to separately specify the registry URL.

sandersn · 2019-10-02T17:47:25Z

src/lib/secrets.ts

@@ -32,6 +32,11 @@ export enum Secret {
     * We only need one token in existence, so delete old tokens at: https://www.npmjs.com/settings/tokens
     */
    NPM_TOKEN,
+    /**
+     * Token used to publish packages to Github.
+     * This *could* be the same as GITHUB_ACCESS_TOKEN, but I think it's better if they remain separate.


I'm not actually sure about this. It might be OK to have one token which can

publish

search for issues

comment on issues

I'll have to think about security a bit before deciding. I think it's fine to start with them separate.

sandersn · 2019-10-02T17:50:41Z

src/publish-registry.ts

+    await writeLog("publish-registry.md", logResult());
+
+    async function publishToRegistry(registryName: "github" | "npm") {
+        const packageName = registryName === "github" ? "@definitelytyped/" + typesRegistry : typesRegistry;


this line, plus the calls to publishToRegistry, are almost all the new code except for registryName in generateJson. Everything else is renames or reordering.

andrewbranch · 2019-10-02T18:00:49Z

src/publish-registry.ts

+            // This may have just been due to a timeout, so test if types-registry@next is a subset of the one we're about to publish.
+            // If so, we should just update it to "latest" now.
+            log("Old version of types-registry was never tagged latest, so updating");
+            await validateIsSubset(await readNotNeededPackages(dt), log);


Does this throw? An error here (or anywhere in this function) publishing to GitHub will stop us from attempting a publish to npm. Not sure whether that’s intentional or not. If the goal is to keep them in sync, that seems good. If the goal is to try publishing to GitHub without effect on current npm infrastructure, that seems bad.

Yes! It does. The intention is to avoid publishing whenever anything goes wrong even if we're not quite sure why, since an old types-registry is way less bad than a new wrong one.

I don't think it's a problem here, but I'll need to inspect the rest of the code.

(I inspected a bunch of code and concluded that there WERE a few problems that got complicated to fix)

I think the safest thing here is to make sure, as long as we're publishing to two places, that github never gets ahead of npm. It's fine if github misses some updates compared to npm, but not the other way around since we always check against npm. That also means, unfortunately, that github needs to update first so that validateIsSubset checks against the current version of npm, not current+1.

I think the right fix is to surround await publishToRegistry('github') with try { ... } catch { }, so that if github fails, npm tries as well.

That was my hunch (that npm should still try to publish even if GitHub fails), but wanted to double check.

sandersn added 6 commits October 1, 2019 08:48

Fix non-dry actions happening during dry run

e8d00b6

github publish works (with many faked/bad changes)

4606de1

Dual publish

f83cacf

Plus cleanup needed to make it readable(ish).

Update jsdoc for secrets

524daf0

Publish to @DefinitelyTyped instead

f9bb495

Publish to DefinitelyTyped not TestTypePublishing

c5dacb2

Also 1. Rename "oldVersion" to "npmVersion" 2. Correctly log why we skipped publishing -- the predicate was backward.

sandersn requested a review from RyanCavanaugh as a code owner October 2, 2019 17:38

sandersn requested a review from andrewbranch October 2, 2019 17:41

sandersn commented Oct 2, 2019

View reviewed changes

andrewbranch approved these changes Oct 2, 2019

View reviewed changes

Fix lint

cac9932

andrewbranch reviewed Oct 2, 2019

View reviewed changes

Compute time-to-update once and ignore errors from github publication

36543e3

sandersn merged commit b70c1e3 into master Oct 2, 2019

sandersn deleted the double-publish-types-registry branch October 2, 2019 21:14

sandersn mentioned this pull request Oct 2, 2019

Publish @‍types Packages to the GitHub Registry microsoft/TypeScript#33330

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Publish types-registry to npm AND github package registry #661

Publish types-registry to npm AND github package registry #661

sandersn commented Oct 2, 2019 •

edited

sandersn commented Oct 2, 2019

sandersn Oct 2, 2019

sandersn Oct 2, 2019

sandersn Oct 2, 2019

sandersn Oct 2, 2019

sandersn Oct 2, 2019

andrewbranch Oct 2, 2019

sandersn Oct 2, 2019

sandersn Oct 2, 2019

sandersn Oct 2, 2019

sandersn Oct 2, 2019

andrewbranch Oct 2, 2019

sandersn Oct 2, 2019

andrewbranch Oct 2, 2019

		}

		private constructor(private readonly client: RegClient, private readonly auth: RegClient.Credentials) {}

Publish types-registry to npm AND github package registry #661

Publish types-registry to npm AND github package registry #661

Conversation

sandersn commented Oct 2, 2019 • edited

sandersn commented Oct 2, 2019

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

sandersn commented Oct 2, 2019 •

edited