-
Notifications
You must be signed in to change notification settings - Fork 40
openssl: delete cgo-less experiment and promote to default #2106
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
Conversation
1afd358 to
0663034
Compare
0663034 to
4207a50
Compare
Patch Consistency Review - Issues FoundI've reviewed PR #2106 for patch consistency and found 3 consistency issues that need to be addressed: Summary of ChangesThe PR is removing the Issues Found
ImpactThese inconsistencies could lead to:
RecommendationPlease update:
I've added specific inline comments on each location that needs attention. The core patch changes (0002 and 0003) look good and are internally consistent with each other.
|
This is part of the go-infra package so we can't remove this here. I also don't think it makes sense to remove it from go-infra for now as we need it for Go 1.26.
I've tried to get the balance right in the migration guide, considering that most users will be running Go 1.26 so mentioning the experiment still makes sense. |
I think the agent needs to be taught that it shouldn't pay attention to vendored Go code in general. |
The comment from the agent was useful to me, it surfaced the need to update the telemetry configuration at some point in the future. The recommendation is wrong, that's true, we don't want to do it now. |
Patch Consistency Review SummaryI've reviewed the patch file changes in this PR, which promotes the cgo-less OpenSSL implementation from an experimental feature ( Changes OverviewThe PR makes consistent changes across:
Issue FoundThere is one inconsistency that needs to be addressed: Patch 0001 (Vendor patch) still contains a reference to RecommendationPlease update the telemetry configuration in patch 0001 to remove the
|
This comment was marked as resolved.
This comment was marked as resolved.
6e135a1 to
1f6a850
Compare
Patch Consistency Review - Incomplete CleanupI've reviewed the patch changes in this PR and found inconsistencies that need to be addressed: 🔴 Issues Found1. Outdated error message in MigrationGuide.md (Line 143)The error message example still references the experiment being removed: This should be removed to maintain consistency with the experiment deletion. ✅ What Was Done Well
📝 Recommended Actions
|
dagood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions, some maybe for followup, otherwise LGTM.
| > This allows the use of OpenSSL without requiring cgo. | ||
| > Currently this experiment is supported on the following architectures: `386`, `amd64`, `arm`, `arm64`, `ppc64le` and `riscv64`. | ||
| > In Go 1.26, there is a cgo-less experiment available for Linux: `ms_nocgo_opensslcrypto`. | ||
| > In Go 1.27, the experiment will be removed and the cgo requirement for `systemcrypto` on Linux will be lifted by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are still on the same line (rendered):
| > In Go 1.27, the experiment will be removed and the cgo requirement for `systemcrypto` on Linux will be lifted by default. | |
| > | |
| > In Go 1.27, the experiment will be removed and the cgo requirement for `systemcrypto` on Linux will be lifted by default. |
Or, should we even mention it here rather than in the other doc? 1.27 doesn't affect anyone yet, so it might not make sense for a migration doc.
| > In Go 1.27, the experiment will be removed and the cgo requirement for `systemcrypto` on Linux will be lifted by default. |
|
Oh, automerge. I guess it's followup then. 😄 |
This is for go 1.27 and onwards