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

Windows binary improvements #1727

Merged
merged 4 commits into from
Nov 24, 2020
Merged

Windows binary improvements #1727

merged 4 commits into from
Nov 24, 2020

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Nov 18, 2020

This enhances the Windows k6 binary by adding an icon and some release metadata like version information, visible in the file's Properties. Additionally, the k6.ico was updated to the new logo in higher resolution, and the MSI installer assets were updated as well.

Before:
2020-11-18-122751_643x654_scrot

After:
2020-11-19-142432_778x648_scrot
2020-11-19-123938_636x500_scrot
2020-11-19-124010_636x509_scrot

This is based on previous work in #1394 and the templating suggestion by @na--. We decided to abandon the templating approach in favor of passing metadata via CLI args.

You can inspect the binary if you download the build artifacts from here.

Pending work:

}

func main() {
ver := strings.SplitN(consts.Version, ".", 3)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be better if consts.Version is not a real constant, but something like []uint16{0, 29, 0}, but that would involve a lot of other changes, and I think we should get this from Git anyway.

@codecov-io
Copy link

codecov-io commented Nov 18, 2020

Codecov Report

Merging #1727 (437fd67) into master (d9bced3) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1727      +/-   ##
==========================================
- Coverage   71.52%   71.50%   -0.02%     
==========================================
  Files         178      178              
  Lines       13719    13727       +8     
==========================================
+ Hits         9812     9816       +4     
- Misses       3295     3299       +4     
  Partials      612      612              
Flag Coverage Δ
ubuntu 71.48% <ø> (+0.01%) ⬆️
windows 70.05% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
core/engine.go 90.95% <0.00%> (-2.02%) ⬇️
stats/cloud/collector.go 79.81% <0.00%> (-0.64%) ⬇️
js/runner.go 81.10% <0.00%> (+0.65%) ⬆️
lib/types/hostnametrie.go 72.22% <0.00%> (+3.47%) ⬆️

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 d9bced3...437fd67. Read the comment docs.

@na--
Copy link
Member

na-- commented Nov 18, 2020

I skimmed through the changes and at glance they LGTM, but to answer your questions:

  • I'm fine with the .msi signing either being in the same PR or in a separate one, whatever you prefer
  • CLI flags for goversioninfo seem slightly preferable - it will make the CI script a bit more verbose, but it'd be much easier to understand what's happening and there would be a lot fewer things that can go wrong
  • not reading the Go Version const is probably fine... you should be able to get the git tag version out of some GitHub Actions environment variable, right?

.github/workflows/all.yml Outdated Show resolved Hide resolved
@imiric imiric mentioned this pull request Nov 18, 2020
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM, I agree with both of you on all the points :D .

I would like to remind people that PR numbers are free (until we are close to a power of 2 ;) ), so having (self-contained) changes spread over multiple PRs IMO is preferable ;)

@imiric
Copy link
Contributor Author

imiric commented Nov 18, 2020

I would like to remind people that PR numbers are free (until we are close to a power of 2 ;) ), so having (self-contained) changes spread over multiple PRs IMO is preferable ;)

True, but MSI package signing is closely related to this, and the less notifications I get and GitHub I have to use, the better :-P (See how I messed up the Assignees and Reviewers in this PR...)

But makes sense, I'll do that in another PR.

Ivan Mirić added 4 commits November 19, 2020 14:27
Converted from the new SVG logo with ImageMagick:

convert -channel rgba -background 'rgba(0,0,0,0)' -define icon:auto-resize=256,128,64,32,16 logo.svg logo.ico
These are now generated by build-release.sh in CI.
@imiric imiric force-pushed the feat/windows-bin-improvements branch from 437fd67 to f1dea93 Compare November 19, 2020 13:28
@imiric imiric requested review from mstoykov and na-- November 19, 2020 13:40
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

LGTM

@imiric imiric merged commit 5091636 into master Nov 24, 2020
@imiric imiric deleted the feat/windows-bin-improvements branch November 24, 2020 09:16
@na-- na-- added this to the v0.30.0 milestone Dec 1, 2020
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.

4 participants