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

x/pkgsite: allow excluding a module/package prefix via a CL #59622

Closed
findleyr opened this issue Apr 13, 2023 · 6 comments
Closed

x/pkgsite: allow excluding a module/package prefix via a CL #59622

findleyr opened this issue Apr 13, 2023 · 6 comments
Assignees
Labels

Comments

@findleyr
Copy link
Contributor

findleyr commented Apr 13, 2023

Background

Right now, the Go tools team (which maintains x/pkgsite) receives on average a couple requests for package removal per week.

These requests follow the protocol outlined in the pkgsite documentation, which recommends first trying to remove a module version using module retraction, and only if that is not a viable solution to file a package removal request.

Handling these requests involves manual intervention from the person on our triage rotation, to comment on the issue, verify the request, and run an exclusion script. Running the script must be done from a Google corp environment that many of us do not regularly use. As a result, these issues constitute a relatively disruptive interrupt for the team.

Furthermore, requesting a removal is pretty onerous for our users: the modules documentation on retraction is sizeable, and posting on the Go issue tracker may be intimidating.

The hope was that module retraction would reduce the amount of toil involved on both sides, but I don't think that has been the case. By far the most common reason for requesting a removal is to remove all versions of a module or package, which cannot easily be achieved with retraction.

Proposal

Therefore, I think we should search for an easier solution. One idea (not mine, but I can't remember whose it was) is to allow users to send a CL to x/pkgsite, to both request and implement the package removal. Specifically:

  • x/pkgsite would have an "exclusions.txt" file containing a list of module prefix exclusions, and perhaps some other metadata (author name, date, reason, etc.)
  • users send a CL adding a line to this file
  • (optional): CI attempts some automatic verification of the user's identity, based on their gerrit email
  • tools team reviews and merges these CLs regularly
  • removal takes effect on the next pkgsite deployment

I think this would avoid a lot of the toil associated with removal requests, since the tools team will need only to approve a CL. Furthermore, it is easier to build automation for CLs than for issues.

Downsides:

  • This file would grow without bound. Counter-argument: the rate of these requests is not high, so it will be a very long time before they become a problem (if ever). At that point in time, we could consider other options.
  • Removal would only occur on next pkgsite deployment. Counter-argument: our current turnaround time in handling these requests is already high enough that I don't think this would affect our latency significantly.
  • Removal would require a Go contribution, which entails a Google account and signing the CLA. This is a barrier for some users; for this reason, we would have to keep the option of posting on the issue tracker. On the other hand, it is easier for a member of the tools team to send a CL themselves than follow the current process.

CC @golang/tools-team

@gophun
Copy link

gophun commented Apr 14, 2023

and posting on the Go issue tracker may be intimidating

And sending a Gerrit CL is not? Would this require a Google account and and signing the CLA as it is required for actual Go contributors?

@gophun
Copy link

gophun commented Apr 14, 2023

x/pkgsite would have an "exclusions.txt" file containing a list of module prefix exclusions

The file name should be "removals.txt" and and not "exclusions.txt", because the listed modules are going to be removed (I hope) and not just excluded.

This file would grow without bound. Counter-argument: the rate of these requests is not high, and we can theoretically compact the file occasionally by moving its contents to some other persistent storage.

Why persistent storage? After they were removed the contents are no longer needed.

@findleyr
Copy link
Contributor Author

And sending a Gerrit CL is not? Would this require a Google account and and signing the CLA as it is required for actual Go contributors?

Yes, adding a new exclusion would be a contribution. For people who don't want to do that, they can still open an issue and a Go team member can send the CL. That is still easier than the current process.

The file name should be "removals.txt" and and not "exclusions.txt", because the listed modules are going to be removed (I hope) and not just excluded.

This mechanism is just for hiding documentation on pkg.go.dev, so I think the name "exclusions.txt" is (approximately) correct.

Why persistent storage? After they were removed the contents are no longer needed.

Sorry, here I was just talking about the file defining the exclusions, not the package contents. This is really not a concern -- the file won't be large -- so I've rephrased. If ever the size of the file becomes a problem, we can revisit.

@gopherbot
Copy link

Change https://go.dev/cl/486458 mentions this issue: internal/worker: allow worker to process dynamic excluded prefix files

@jamalc jamalc modified the milestones: Unreleased, pkgsite/later Apr 28, 2023
@jamalc jamalc self-assigned this Apr 28, 2023
gopherbot pushed a commit to golang/pkgsite that referenced this issue May 4, 2023
For golang/go#59622.

Change-Id: I4059df405e248824a3521fcc084568b052fe4380
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/486458
Reviewed-by: Suzy Mueller <suzmue@golang.org>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Jamal Carvalho <jamal@golang.org>
@jamalc jamalc closed this as completed May 17, 2023
@jamalc
Copy link

jamalc commented May 17, 2023

We've simplified the internal process, closing this for now.

@adonovan
Copy link
Member

Nice work! Many thanks @jamalc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants