There's no way to declare that module is auto generated about dt-mergebot HOT 15 CLOSED

I'm getting a feeling that automatically generated packages [...] are not welcome on DT. Or at least that DT is not the right place for them.

The work to allow peer reviewed merges helps the auto-generated PRs too, we've put hundreds of human hours to get all this infra working in the last few months - it's already an improvement over 2019 DT. We care.

Historically, what we've done is give some of the people who ship large generated packages commit access to DT - ideally that's not our solution, but it has worked before.

Personally, an option could be that we make an allow list in the repo of DT packages which are auto-generated packages. Folks can apply via a PR to that text file. If a PR comes in which only affects a single module in that list, then we allow anyone in the DT types header for that package to declare 'ready to merge' instead of the submitter like today.

That allows for:

• auto-generated PRs from a bot
• a human account reviews it (or rubber stamps, but it's their responsibility)
• they can ask us a DT maintainers if they have questions

This balances responsibilities pretty well I think, we get all the CI validation etc and have a chance for oversight if needed - automated authors get the ability to read their PR and then call it good to go

from dt-mergebot.

So, for this case.

You'd change the account which generates the DT PR to be something like "google-api-types-bot". Then because your Maxim GH account is already in the .d.ts header files you would OK to do a positive review and then instead of the bot saying:

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

It would say something like

All of the items on the list are green. To merge this PR, @Maxim-Mazurok (and anyone else with access) needs to post a comment including the string "Ready to merge" to bring in the changes.

I need to at least get consensus internally that this is cool, but that's what I mean would happen - which I believe is what @ExE-Boss is talking about too 👍

from dt-mergebot.

@Maxim-Mazurok, what @orta suggests is that you create a bot account that will do the PRs (which is in line with what some other generated packers do). This will have two benefits: (a) It will be very clear that it's an automated PR (since it will come from some *-bot username; (b) your own account will still be the owner, and therefore you will be able to approve these PRs and self merge them.

(BTW, I thought that I mentioned a bot doing the PRs earlier as a conventional way for doing automated PRs, but I see that I didn't do that explicitly... Sorry about that.)

from dt-mergebot.

(I forgot earlier that only the PR author can ask for a merge, but #156 makes it possible for owners to do so too.)

from dt-mergebot.

First, I made changes to the mergebot which would make merging bugs easier in trivial cases.

Second, I ran into one of these PRs, and I think that something is wrong with the process you're using to update this thing. Whatever it is, it sounds wrong to me that there are so many updates for each of these PRs. Perhaps it's better to commit to a single change and wait until it's merged in before re-generating the PR which leads to nullifying reviews. If the process is continuous, then you're basically (ab)using DT as a publishing platform for a constantly moving target, and that leads to wasting human efforts in the form of reviewing the changes...

However, having gone over the PR you mentioned now (I think it's the one that I've seen previously), the force-pushes were all just changing the // Revision: line at the top -- and given that git is intended to track revisions, maybe things would automatically become much better if you just completely drop that line from the code?

Otherwise, if you really think that it's needed, then a common practice with publishing generated code is to create the new code, then compare it to the previous version ignoring things like that revision line, and if the result is the same, then do nothing. Note that I'm talking about the build process here, not the update pushing. So instead of the process going like

metadata source --gen--> index.d.ts

it will be:

metadata source --gen--> index.new.d.ts --move--> index.d.ts

where the last step is done only if there are changes between index.new.d.ts and index.d.ts, excluding the revision line.

(Oh and BTW, I disagree with having an exception for blindly accepting auto-generated PRs... The fact that it's autogenerated shouldn't prevent people from reviewing it and pointing at problems. If anything, then it's even more important since some trivial bug in the autogen process can end up with a blank file, and if such a PR is blindly accepted it'll make your users very sad.)

from dt-mergebot.

First, I made changes to the mergebot which would make merging bugs easier in trivial cases.

Thanks! Do you mean this one #131?

Second, I ran into one of these PRs, and I think that something is wrong with the process you're using to update this thing. Whatever it is, it sounds wrong to me that there are so many updates for each of these PRs. Perhaps it's better to commit to a single change and wait until it's merged in before re-generating the PR which leads to nullifying reviews. If the process is continuous, then you're basically (ab)using DT as a publishing platform for a constantly moving target, and that leads to wasting human efforts in the form of reviewing the changes...

That's exactly what I don't want to do: waste reviewers time or abuse DT.

However, having gone over the PR you mentioned now (I think it's the one that I've seen previously), the force-pushes were all just changing the // Revision: line at the top -- and given that git is intended to track revisions, maybe things would automatically become much better if you just completely drop that line from the code?

It's a good point. But we need to have a revision number that comes from Google because their API is not consistent, meaning that on first request I get 2020-01-01 revision, on the second one - 2020-10-10, and on the third one 2020-01-01 again. I believe that this is caused by the load-balancing of some sorts. And because we don't want to go back and forth regenerating d.ts - we're using revisions to check if it's newer than what we already have. Also, as you can see we're not opening new PRs if only revision changed. But rather updating existing PRs in that case.
Clearly, this still causes issues with current merging process, so I can see two potential fixes:

• Store revision numbers internally in meta-file, not published to DT.
• Don't update PRs at all, only open new ones. Perhaps, only when there's no existing PRs for that type.

Otherwise, if you really think that it's needed, then a common practice with publishing generated code is to create the new code, then compare it to the previous version ignoring things like that revision line, and if the result is the same, then do nothing. Note that I'm talking about the build process here, not the update pushing. So instead of the process going like

metadata source --gen--> index.d.ts

it will be:

metadata source --gen--> index.new.d.ts --move--> index.d.ts

where the last step is done only if there are changes between index.new.d.ts and index.d.ts, excluding the revision line.

Yes, it looks like a viable option.

(Oh and BTW, I disagree with having an exception for blindly accepting auto-generated PRs... The fact that it's autogenerated shouldn't prevent people from reviewing it and pointing at problems. If anything, then it's even more important since some trivial bug in the autogen process can end up with a blank file, and if such a PR is blindly accepted it'll make your users very sad.)

Our types are thoroughly tested before publishing, but I can see that others might not have this testing in place.
Perhaps, we could publish unreviewed changes to package@beta? And then once they are reviewed - it can be promoted to package@latest?

Otherwise, I can create a dummy GitHub account, make it the second owner of types and automatically approve all PRs to DT. This is hacky, but the fact that it will work means that either approval process should be more stict (to accept reviews only from verified trusted users), or that it can be simplified (instead of creating dummy account for approvals, some flag could be added to the mergebot).

Let me know what you think about this, thanks!

from dt-mergebot.

First, I made changes to the mergebot which would make merging bugs easier in trivial cases.

Thanks! Do you mean this one #131?

#99, ... #151

• Store revision numbers internally in meta-file, not published to DT.
• Don't update PRs at all, only open new ones. Perhaps, only when there's no existing PRs for that type.

Yes, I realize that the first is not great since it would make it depend on files that you maintain elsewhere, which is why I think that the second is better: if the resulting code is the same as the current one in a PR (without the revision line), then consider it to be the same so no updates would happen. When a PR is merged, it would similarly avoid openning new PRs until there are additional updates that cause changes in the same way.

Our types are thoroughly tested before publishing, but I can see that others might not have this testing in place.

Well... As I keep telling students in my class: when you have a type system with proper unions and therefore proper subtypes, you get a choice of the types you want since in many cases there's no single correct choice. This means that while testing is always good, people might have good feedback about the types you choose. This is even more true in the case of typescript, where one of the considerations for choosing types is the quality of support you get for completions from the tsserver.

Perhaps, we could publish unreviewed changes to package@beta? And then once they are reviewed - it can be promoted to package@latest?

Practically speaking, if you'd update such a side package in the same frequency due to revision dates, you'll run into the same problem...

Otherwise, I can create a dummy GitHub account, make it the second owner of types and automatically approve all PRs to DT. This is hacky, but the fact that it will work means that either approval process should be more stict (to accept reviews only from verified trusted users), or that it can be simplified (instead of creating dummy account for approvals, some flag could be added to the mergebot).

Yes, you can game the system, but there is a strong element of trust that the whole thing relies on. If there's a group of people that work with you, then maybe convince someone else to become an owner too, and at least have a bried look at PRs to see that the changes look fine?

from dt-mergebot.

people might have good feedback about the types you choose

Since the process is automated on our side, I think that the proper way to give feedback on our types choice is by opening issue/PR to the repo which generates types.

Perhaps, we could publish unreviewed changes to package@beta? And then once they are reviewed - it can be promoted to package@latest?

Practically speaking, if you'd update such a side package in the same frequency due to revision dates, you'll run into the same problem...

Which problem, exactly?.. I think it's fine to have a lot of frequent nightly releases and fewer stable releases.
It could work like this: any PR opened with [x] Beta flag will be treated as fast-tracked, auto-merged to beta branch and published to @beta if tests pass. And our bot will go nuts, opening PRs as frequent as it needs to.

And then, it will also open "stable" PRs with [ ] Beta flag unchecked. They will be "shoot and forget" PRs, that bot will not update and patiently waiting for it to be merged to master and published as stable.

I understand that this may be time consuming to code, but I'm happy to do this after discussion.

Yes, you can game the system, but there is a strong element of trust that the whole thing relies on. If there's a group of people that work with you, then maybe convince someone else to become an owner too, and at least have a brief look at PRs to see that the changes look fine?

Actually, since theses PRs are automatic, I'm usually not aware of code changes. So, when I open PR - I don't know if it's good or bad. So, technically, it's just like someone else's PR to me. If I could review it - that would be great. GitHub doesn't allow author to review PR, but I think that some command for merge-bot could solve this.
Unfortunately, other contributors are not very active on GH, but it's still a good idea and we have an issue open to do that.

Slightly off-topic: I wrote a tiny paper about Trust Issues In Open Source and NPM a while ago, might be an interesting read.

from dt-mergebot.

people might have good feedback about the types you choose

Since the process is automated on our side, I think that the proper way to give feedback on our types choice is by opening issue/PR to the repo which generates types.

As a reviewer on DT, people will probably not care about the process, but will about the end result. This means that if there's some choice of type that is questionable, you'll probably get a note about that end product which you'll need to translate to tweaks for the generation.

Practically speaking, if you'd update such a side package in the same frequency due to revision dates, you'll run into the same problem...

Which problem, exactly?..

Discouraging reviews by the fact that the side package would be updated multiple times every day, outdating reviews quickly.

I think it's fine to have a lot of frequent nightly releases and fewer stable releases.
It could work like this: any PR opened with [x] Beta flag will be treated as fast-tracked, auto-merged to beta branch and published to @beta if tests pass. And our bot will go nuts, opening PRs as frequent as it needs to.

Again, having such an exception on DT is impractical. How could the bot (or humans) identify the validity of such flags? If it's a whitelist, then it's essentially equivalent to you asking for an exception and just admit all of your PRs automatically, and that's unlikely to happen. Or if you're talking about the DT side keeping track of "beta" packages that are approved like that, then that's a bunch of machinery which would be a huge load on an already gigantic repository. (Gigantic in all aspects, most importantly the amount of traffic.) So again, unlikely to happen.

I understand that this may be time consuming to code, but I'm happy to do this after discussion.

Well, the bot is here, so you can do the work and suggest the change, but my guess is that people won't want that.

Actually, since theses PRs are automatic, I'm usually not aware of code changes.

That's a good indication that your process has a problem. If it all happens automatically, then it's not really code that you're publishing, it's more like a compilation artifact. If the code generation is automated enough, it might make sense to publish it differently: maybe a tool that does the codegen when you first import it (there are existing tools that generate type definition, one which is used by the bot is apollo, generating types for GQL queries).

Slightly off-topic: I wrote a tiny paper about Trust Issues In Open Source and NPM a while ago, might be an interesting read.

(One thing that you should mention more explicitly there is popularity. The types for event streams, for example, has MANY people looking at it for that reason.)

from dt-mergebot.

Thank you for your opinion.

I'm getting a feeling that automatically generated packages (even high-quality and for such a popular thing as Google client API) are not welcome on DT. Or at least that DT is not the right place for them.
Unfortunately, I can't do my own auto-publishing into @types scope, since it's owned by DT. And I don't want to use another scope because @types is the industry standard.

I think that the best solution is to "game the system" by approving all of my automatic PRs instantaneously using separate account. This way I'll achieve my goal of keeping types up to date, and DT reviewers won't have to waste time reviewing these changes. The only drawback is having more commits in DT repo, but with "no revision-only updates" I'll keep number of PRs reasonably low.

It would be great to get some input from @orta on that matter as well.

from dt-mergebot.

That conclusion is definitely wrong. There are many packages that are generated.

from dt-mergebot.

That conclusion is definitely wrong. There are many packages that are generated.

It wasn't a conclusion, it was a feeling :)
I should've been more specific: automatically generated and automatically updated packages.

The work to allow peer reviewed merges helps the auto-generated PRs too, we've put hundreds of human hours to get all this infra working in the last few months - it's already an improvement over 2019 DT. We care.

Thank you! 🤗

an option could be that we make an allow list in the repo of DT packages which are auto-generated packages

That sounds great, actually! I would love to help implementing this if there're no objections.

But I don't think that I understand this part:

then we allow anyone in the DT types header for that package to declare 'ready to merge' instead of the submitter like today

As far as I understand, that's how it works today (I need some other owner to approve PR), but what I want is to be able to declare it 'ready to merge' by myself (submitter "approves" PR using some bot command).
Is that what you meant?

Matter of fact, I do review all of my automated PRs just in case, when bot comments on it and I get notification. It would be great if I could "approve" it.

from dt-mergebot.

It might also be a good idea to make it possible to mark some owners as allowing the “ready to merge” comment to be made by another owner, that way, non‑automated PRs won’t be auto‑merged because of non‑author “ready to merge” comments.

from dt-mergebot.

It might also be a good idea to make it possible to mark some owners as allowing the “ready to merge” comment to be made by another owner, that way, non‑automated PRs won’t be auto‑merged because of non‑author “ready to merge” comments.

AFAIK, in this case another owner can just approve PR and it will be auto-merged (if all tests passing, etc.).
I think that "ready to merge" comment can be described as a workaround to GH limitation of not allowing authors to approve PRs.

from dt-mergebot.

Thanks, @orta and @elibarzilay

Making PRs from a bot account will take some credit from my main account, but I guess I can live with that.
This solution sounds like the most reasonable one, so I'll go with it.

from dt-mergebot.