Definitelytyped: Unacceptable automation/sockpuppeting: gapi via google-api-typings-generator account

Created on 29 Oct 2020  路  23Comments  路  Source: DefinitelyTyped/DefinitelyTyped

@Maxim-Mazurok The automation you've put in place here isn't keeping with the spirit of the repo - the clear intent of the process is that two humans have been involved in any change, not a human operating two accounts, and definitely not two bots. Running a separate bot account is effectively sockpuppeting to get around the rules. This has to immediately cease.

There are a few different problems here:

  • As noted above, the blatant process circumvention. This is setting a bad precedent for other users and increasing risk that we publish something inadvertent to @types
  • The sheer volume of PRs, including "revision-only" ones like this, is very noisy and is consuming an inordinately large share of our resources

I'm offering the following two options, either of which would be fine:

  • Batch these into a more reasonable cadence (weekly) along with detection to not push "revision-only" PRs
  • Host the definition files on a different npm package; we will allowlist these in the typings-publisher and convert the @types/ packages to "pass-through" packages that just depend on the external package

I've put an organizational block on the google-api-typings-generator account in the interim while we figure out the best path forward. Thanks for helping us figure out a more sustainable solution.

Most helpful comment

I'd prefer to make this a one-off case since I think it is fairly rare.
I think the right thing is to replace the gapi.* code on DT with the passthrough @andrewbranch mentioned instead of deprecating it.

Since gapi.client.sheets is already deprecated, it will be little more complicated to bring it back, but I think just re-creating the directory and basic file structure should work. Oh, and the entry from notNeededPackages will need to be deleted. I'm not sure how CI will react to that, but I think it'll be fine.

All 23 comments

Hello, Ryan. I'm very sorry that this process caused any issues.

It was never my intent to be noisy, consume too much of your resources, or be a bad actor in this repo in any way.

I did discuss this with @orta, @elibarzilay, and other maintainers previously and I thought that my intent for automation was always clear and acceptable. I can see now that it's not the case anymore.

I went ahead and

  • disabled the workflow responsible for opening PRs automatically.
  • Revoked all the tokens for @google-api-typings-generator account
  • Updated description of @google-api-typings-generator account. Let me know if I should delete this account, or leave for historical purposes?

We do have logic to prevent revision-only PRs, but it clearly has a bug in it. I didn't pay too much attention to it, since the majority of PRs are merged automatically and I didn't expect them to consume any of your resources for that reason.

Moving forward, I think that this approach is the best one:

Host the definition files on a different npm package; we will allowlist these in the typings-publisher and convert the @types/ packages to "pass-through" packages that just depend on the external package

As it will let me provide the latest and accurate type definitions, while not consuming too much of your or my resources.
We can specify @latest in the package.json of @types/gapi.client.* packages and remove package-lock.json so that there will be no need to make PR to DT with an update every time a new version of type definitions is published, right?

Thank you so much for understanding and collaboration!

Thanks for the quick response and understanding on this one! I spoke to the other maintainers and we didn't quite realize what we were signing up for on those previous conversations, and we'll try to be more clear in the future.

The passthrough option sounds great and should be straightforward to merge in on the DT side. Let me know if we can do anything to help.

that there will be no need to make PR to DT with an update every time a new version of type definitions is published, right?

Correct; you can refer to things in notNeededPackages.json to see how this is done for other similar packages

Thank you for your support, I'm happy that we're all now on the same page :)

In https://github.com/Maxim-Mazurok/google-api-typings-generator/pull/392 and some other commits, I removed PR-opening logic and replaced it with NPM publishing logic.

Now we're publishing some "pilot" gapi.client.* packages to NPM with the same major and minor versions, but we're using revision numbers from Google as a patch version, which was always my dream :)

I've opened https://github.com/DefinitelyTyped/DefinitelyTyped/pull/49299 to test this approach on one of the packages, before scaling it to other packages. Please, take a look when you have a chance, and I'll be happy to get your feedback on this!

So, #49299 was merged and I'm testing @tyles/gapi.client.sheets.

First, when installing it, I'm getting a misleading deprecation notice:

npm WARN deprecated @types/gapi.client.[email protected]: This is a stub types definition. gapi.client.sheets provides its own type definitions, so you do not need this installed.

It's misleading because people do need it installed and shouldn't remove this from package.json.

Another issue that I'm having is that now TS can't find namespace declared in this package:

Namespace 'gapi.client' has no exported member 'sheets'.

I've tried this, no luck: "typeRoots": ["node_modules/@types", "node_modules/gapi.client.sheets"]

I also tried to add "types": "index.d.ts" to the package.json of the installed gapi.client.sheets package, no luck either.

Here's the project that I'm using for the test: https://github.com/Maxim-Mazurok/expenses

Any help in resolving this would be greatly appreciated because without fixing this we can't move forward :(

I managed to solve this by using import 'gapi.client.sheets'; in the project that uses these type definitions.
But this is not ideal, the original idea was to have the global namespace.

@sandersn thoughts on making an option for notNeededPackages that better addresses this use case? It would need to

  • not run npm deprecate
  • add a types field and index.d.ts with /// <reference types="{source library name}" />

The other option would be to leave this stub in DefinitelyTyped (which is actually the approach I assumed would be taken here before fully catching up on this thread)

I'd prefer to make this a one-off case since I think it is fairly rare.
I think the right thing is to replace the gapi.* code on DT with the passthrough @andrewbranch mentioned instead of deprecating it.

Since gapi.client.sheets is already deprecated, it will be little more complicated to bring it back, but I think just re-creating the directory and basic file structure should work. Oh, and the entry from notNeededPackages will need to be deleted. I'm not sure how CI will react to that, but I think it'll be fine.

I agree with that assessment. @Maxim-Mazurok, does that make sense?

Thanks for the feedback! Sounds good, so I'm going to open PR that:

  • Removes gapi.client.sheets from notNeededPackages.json
  • Adds `gapi.client.sheets folder with single index.d.ts file that contains /// <reference types="{source library name}" />

I'm not sure how I can do this though:

  • not run npm deprecate

I guess it won't run if I remove the package from notNeededPackages.json. But I'm not sure how to "undeprecate" it now...

THe publisher will publish a new version which will not be deprecated. There will still be one version that's deprecated, but nobody will get it if they install @latest.

The index.d.ts will need to do what @andrewbranch said, and /// <reference types="gapi.client.sheets" />, plus reference the package in package.json. That way when people install @types/gapi.client.sheets, they'll get all the types from gapi.client.sheets (or whatever the actual package will be called).

Please, check out this PR: #49347

I had to add other files: tslint.json and tsconfig.json in order to satisfy tests.

And then I had to add DT header to the index.d.ts, then I had to remove types from package.json.

And after all, I still get the error: A file cannot have a reference to itself.

Because I published a package with the name gapi.client.sheets, so reference doesn't work as expected...
What's the best way to solve this? Should I rename my package to something else? I can't change the namespace though.

Also, please take a look at https://github.com/microsoft/DefinitelyTyped-tools/pull/154

So that I can list gapi.client.sheets as a dependency.

Oh no. I should have thought about the name conflict. The namespace should be fine, because <reference types='x' /> is still a file/folder lookup for 'x'. I think renaming gapi.client.sheets to something else will work.

Thank you for a quick response, I re-published it under @maxim_mazurok/gapi.client.sheets name.
Fixed #49347
And waiting for https://github.com/microsoft/DefinitelyTyped-tools/pull/156 to be merged

https://github.com/microsoft/DefinitelyTyped-tools/pull/156 got merged

https://github.com/DefinitelyTyped/DefinitelyTyped/pull/49347 passed all the checks, waiting for it to be merged to test how it works with gapi.client.sheets again, before scaling this approach to other types as well.

49347 got merged, but I'm not sure if and when the @types/gapi.client.sheets package will be published again...

The publisher hit an error due to the flip-flopping of the deprecation, and required manual intervention鈥擨 believe @sandersn has now fixed it 馃帀

It worked, thanks!
I tested out new typings on my project and it worked just fine. I'm going to open PRs to do the same thing for other @types/gapi.client.* types as well.
Can I do this in bulk for all @types/gapi.client.* packages, or it's better to open one PR per package at a time?

@weswigham it's up to you since you're on this week. I'd personally prefer the bulk change since it's not like we need to pull in reviewers for each package.

馃し whatever works seems fine

Heads up: looking at the last overnight test run, I see 7 failures of the lint rule npm-naming on gapi.client.* because @types/gapi.client.* packages claim to be non-npm packages in their header, but npm nonetheless has a package with the name.

@Maxim-Mazurok I guess that's from the packages you created before moving them into the @maxim_mazurok scope, right?

There are two ways to fix this:

  1. I can create lint rule exceptions for these packages.
  2. You can delete these packages.

Here are the packages that are failing right now:

  • gapi.client.analytics
  • gapi.client.analyticsreporting
  • gapi.client.calendar
  • gapi.client.classroom
  • gapi.client.discovery
  • gapi.client.drive
  • gapi.client.storage

Let me know whether you want me to exempt these @types packages from the rule, or whether you want to delete these packages.

You're right, @sandersn, sorry for the inconvenience, I just unpublished them.

I can't see gapi.client.sheets in your list, but I think it should cause the same issues. I can't unpublish it yet, because it has too many downloads (npm doesn't let me), so I deprecated it for now.

Hello guys, sorry for the delay, I just finished opening PRs to DT and to DT-tools:

Please, take a look when you have a chance, thanks!

Was this page helpful?
0 / 5 - 0 ratings