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

chore: use go build constraint #289

Merged
merged 7 commits into from
May 29, 2023
Merged

Conversation

crazy-max
Copy link
Member

fixes #176

Switch to go build constraint. This allows to run tests against the right architecture. Errors in pass tests are now properly handled.

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2023

Codecov Report

Patch coverage has no change and project coverage change: -1.94 ⚠️

Comparison is base (a652f8e) 54.68% compared to head (492a7b1) 52.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #289      +/-   ##
==========================================
- Coverage   54.68%   52.74%   -1.94%     
==========================================
  Files           9        9              
  Lines         673      673              
==========================================
- Hits          368      355      -13     
- Misses        262      271       +9     
- Partials       43       47       +4     
Impacted Files Coverage Δ
osxkeychain/osxkeychain.go 69.10% <ø> (ø)
secretservice/secretservice.go 0.00% <ø> (ø)
wincred/wincred.go 78.35% <ø> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@crazy-max
Copy link
Member Author

crazy-max commented May 28, 2023

@thaJeztah I just noticed that this would solve #277 as well

@crazy-max crazy-max requested a review from thaJeztah May 28, 2023 19:14
@crazy-max crazy-max marked this pull request as ready for review May 28, 2023 19:14
osxkeychain/osxkeychain_test.go Outdated Show resolved Hide resolved
pass/pass.go Outdated
@@ -1,3 +1,6 @@
//go:build linux || darwin
Copy link
Member

Choose a reason for hiding this comment

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

slightly on the fence on this one; I think the code in this repo works fine on any platform (if there's a pass binary to shell out to?)

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, switched to unix.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually !windows looks better.

Copy link
Member

Choose a reason for hiding this comment

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

So for the pass helper; I guess the thing I was wondering here was that;

  • the code in this repository (the helper itself) is basically platform-agnostic (including Windows)
  • the "only" limitation we have for testing is that we don't have a pass binary on Windows
  • so, wondering if we should actually exclude it on Windows?
  • (but indeed, we can't currently run the tests, as those depend on a pass binary)
  • ^^ do we know if there's an implementation of the pass binary for windows?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like there's at least one implementation (although last release is really old; https://github.com/mbos/Pass4Win)

Copy link
Member Author

@crazy-max crazy-max May 29, 2023

Choose a reason for hiding this comment

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

Tried to install Pass4Win in ci but binary is not named pass.exe 😣 https://github.com/docker/docker-credential-helpers/actions/runs/5110526809/jobs/9186513917#step:9:8

So disable tests for windows in the meantime. Let me know if you prefer a specific tag like exclude_pass_tests that would be just set in CI.

pass/pass_test.go Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member

Look like it timed out or so 😔

@crazy-max crazy-max force-pushed the build-constraint branch 2 times, most recently from 399b954 to 492a7b1 Compare May 29, 2023 09:33
@crazy-max crazy-max requested a review from thaJeztah May 29, 2023 09:36
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max
Copy link
Member Author

@thaJeztah PTAL when you can 🙏, would like to rebase #288

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 83d38ea into docker:master May 29, 2023
10 checks passed
@crazy-max crazy-max deleted the build-constraint branch May 29, 2023 16:13
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.

Problem when cross-compiling on Linux for OSX
3 participants