Skip to content

packit: set default for GOPROXY #86

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

schuellerf
Copy link
Contributor

@schuellerf schuellerf commented Jan 21, 2025

The default in our CI is "direct" which is not as stable as the more common https://proxy.golang.org,direct.

This also moves the implementation to the Makefile to have it more central.

@supakeen
Copy link
Member

I'll be deferring review of this as we seem to be in gear to replace most of our vendoring scripts/Makefiles with go-vendor-tools as proposed by the Go SIG in Fedora. We plan to discuss this tomorrow.

@mvo5
Copy link
Collaborator

mvo5 commented Jan 21, 2025

Setting to "blocked" based on #86 (comment) - please just remove the label once that discussion has happend

@schuellerf schuellerf force-pushed the move-goproxy-to-makefile branch 2 times, most recently from 33a93a7 to e475a4c Compare January 21, 2025 14:26
@supakeen
Copy link
Member

supakeen commented Mar 1, 2025

Since we don't seem to really have time to look into go-vendor-tools I'll unblock this and we can decide here.

@schuellerf schuellerf force-pushed the move-goproxy-to-makefile branch from e475a4c to 71632cd Compare March 3, 2025 10:08
mvo5
mvo5 previously requested changes Mar 4, 2025
Copy link
Collaborator

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

I think this should be closed based on the reasoning in https://github.com/osbuild/image-builder-cli/pull/86/files#r1923834932

If not closed it should follow upstream and use GOPROXY=https://proxy.golang.org,direct - disabling the "gatekeeping" feature would mean if there is a malicious go module we would not get protection from the upstream go proxy (i.e. the default behavior allows the upstream to deny-list known malicious modules which we would disable by using "|" instead of ",").

@schuellerf
Copy link
Contributor Author

I'm ok with GOPROXY=https://proxy.golang.org,direct this would also improve robustness of CI.
(which basically is now there for packit exactly for this reason)

@schuellerf schuellerf force-pushed the move-goproxy-to-makefile branch 2 times, most recently from 96b15aa to 63bafa7 Compare March 4, 2025 16:46
@schuellerf schuellerf changed the title packit: setup GOPROXY without 'gatekeeping' packit: set default for GOPROXY Mar 4, 2025
@mvo5
Copy link
Collaborator

mvo5 commented Mar 4, 2025

I'm ok with GOPROXY=https://proxy.golang.org,direct this would also improve robustness of CI. (which basically is now there for packit exactly for this reason)

I think its fine to set the GOPROXY in our CI but not in the user facing Makefile.

On a fedora system this change will override the system default behavior related to the module loading (which can be inspected via go env GOPROXY and (which is a the go package default not an environment that can be checked via the Makefile). This will lead to inconsistent behavior between running "go build" and "make". I strongly feel it is not our call to make to override the systems choices in our Makefile - either the user decides to override the system defaults on their own for everything or they don't but we should not make this decision (we could try to convince fedora to avoid this change).

@achilleas-k
Copy link
Member

I strongly feel it is not our call to make to override the systems choices in our Makefile - either the user decides to override the system defaults on their own for everything or they don't but we should not make this decision (we could try to convince fedora to avoid this change).

Fully agree with this part especially (and the rest of the comment more generally).

The default in our CI is "direct" which is not as
stable as the more common `https://proxy.golang.org,direct`.
This also moves the implementation to the Makefile
to have it more central.
@schuellerf schuellerf force-pushed the move-goproxy-to-makefile branch from 63bafa7 to a5d5b85 Compare March 4, 2025 18:15
@schuellerf schuellerf requested a review from mvo5 March 6, 2025 08:28
@schuellerf schuellerf dismissed mvo5’s stale review March 7, 2025 08:54

please check the changed implementation

@supakeen supakeen removed their request for review March 11, 2025 08:21
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

Thank you!

Would it be possible to move go mod vendor into rpm_spec_add_provides_bundle.sh? Since it's a clear prerequisite for the generation process, it imho makes more sense to group these together.

Then we either don't need the makefile target at all, or we can just call it the same as the script: make rpm_spec_add_provides_bundle (I have a bit of underscores vs. dashes dilemma here 😅 ).

Also, please make sure that the commit message, and the PR title match what the change is actually about. Thank you! ❤️

@achilleas-k
Copy link
Member

What's the status of this? Let's make a decision and either finish it or close it.

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.

5 participants