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

Follow-up fixes for #996 #1003

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eskultety
Copy link
Member

  • follow good practice of not using a list as a default argument due to immutability in integration tests
  • drop most explicit GOSUMDB=off assignments and only set it to its default value for resolve_gomod

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • New code has type annotations
  • OpenAPI schema is updated (if applicable)
  • DB schema change has corresponding DB migration (if applicable)
  • README updated (if worker configuration changed, or if applicable)
  • Draft release notes are updated before merging

Making an empty list a default argument is dangerous because lists are
mutable. Luckily for us the argument isn't being updated in the
function. Still, a tuple is the suggested approach here.

While at it, add a doc string entry for the argument in question.

Signed-off-by: Erik Skultety <[email protected]>
We should only really need to set GOSUMDB explicitly with the module
fetch so that we can verify e.g. a toolchain that was downloaded as a
result of using GOTOOLCHAIN=auto in resolve_gomod.
Every other occurrence where we explicitly disable GOSUMDB is
redundant. As for hermetic (i.e. network isolated) builds, as long
GOMODCACHE is provided, the sum database should not be needed. As far
as additional toolchains are concerned with regards to checking the sum
database during offline project builds, setting the GOPROXY variable to
point to the correct location on disk within GOMODCACHE should be
enough to pass any kind of verification.

Signed-off-by: Erik Skultety <[email protected]>
@eskultety eskultety changed the title Follow-up fixes for !996 Follow-up fixes for #996 May 29, 2024
@eskultety eskultety marked this pull request as ready for review May 29, 2024 11:17
@eskultety eskultety requested review from taylormadore and brunoapimentel and removed request for taylormadore May 29, 2024 11:17
Copy link
Member

@ben-alkov ben-alkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (but IANAE)

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

Successfully merging this pull request may close these issues.

2 participants