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

go1.18: Preserve trailing tabs while massaging go version -m output #668

Merged
merged 2 commits into from
Mar 22, 2022

Conversation

tstromberg
Copy link
Contributor

@tstromberg tstromberg commented Mar 22, 2022

Allows ko on both go v1.17 and v1.18 to parse go version -m output that contains a local replace directive. For example:

./main: go1.18
	path	command-line-arguments
	dep	secret.dev/api	v0.0.0-00010101000000-000000000000
	=>	../api	(devel)	
	

The issue is that ParseBuildInfo assumes there is a 3rd column that contains a checksum, which is not applicable to local replace directives. This patch inserts the extra tab, allowing it to have an empty checksum.

Fixes #666

jonjohnsonjr
jonjohnsonjr previously approved these changes Mar 22, 2022
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

Can we add this to massageBuildInfo?

@imjasonh
Copy link
Member

@tstromberg
Copy link
Contributor Author

Ooh, I didn't notice massageBuildInfo before. Not only is the perfect place to implement this, but uhh.. it apparently the source of the bug too! I'll revise the patch.

@imjasonh
Copy link
Member

Ooh, I didn't notice massageBuildInfo before. Not only is the perfect place to implement this, but uhh.. it apparently the source of the bug too! I'll revise the patch.

Oh no. 😭

@tstromberg tstromberg changed the title go1.18 compatibility: Add workaround for local replace directives go1.18: Preserve trailing tabs while massaging go version -m output Mar 22, 2022
@tstromberg
Copy link
Contributor Author

tstromberg commented Mar 22, 2022

Thanks again for the massageBuildInfo hint.

This PR has been updated to address the root cause instead of putting lipstick on a pig.

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉

@codecov-commenter
Copy link

Codecov Report

Merging #668 (256f31a) into main (327a88f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #668   +/-   ##
=======================================
  Coverage   51.62%   51.62%           
=======================================
  Files          44       44           
  Lines        3268     3268           
=======================================
  Hits         1687     1687           
  Misses       1371     1371           
  Partials      210      210           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 327a88f...256f31a. Read the comment docs.

@imjasonh imjasonh merged commit cefd28f into ko-build:main Mar 22, 2022
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.

Regression in v0.11.1: Fails with could not parse Go build info
4 participants