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

Return basic auth from google.Keychain #791

Merged
merged 1 commit into from
Oct 22, 2020

Conversation

jonjohnsonjr
Copy link
Collaborator

Before, we were taking advantage of the fact that GCR will allow you to
just send an access token as bearer auth, but apparently this breaks
gcrane cp if it's your first push to a registry, so this change
returns basic auth to force us to always do a token exchange.

@jonjohnsonjr
Copy link
Collaborator Author

FYI @hosungs

Copy link

@hosungs hosungs left a comment

Choose a reason for hiding this comment

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

LGTM

Before, we were taking advantage of the fact that GCR will allow you to
just send an access token as bearer auth, but apparently this breaks
`gcrane cp` if it's your first push to a registry, so this change
returns basic auth to force us to always do a token exchange.
@codecov-io
Copy link

codecov-io commented Oct 22, 2020

Codecov Report

Merging #791 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
- Coverage   75.55%   75.54%   -0.02%     
==========================================
  Files         102      102              
  Lines        4169     4171       +2     
==========================================
+ Hits         3150     3151       +1     
- Misses        565      566       +1     
  Partials      454      454              
Impacted Files Coverage Δ
pkg/v1/google/auth.go 75.00% <100.00%> (+0.53%) ⬆️
pkg/crane/options.go 69.23% <0.00%> (-5.77%) ⬇️

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 8e0bbb8...c686a8b. Read the comment docs.

@jonjohnsonjr jonjohnsonjr merged commit 8a28419 into google:master Oct 22, 2020
@jonjohnsonjr jonjohnsonjr deleted the google-auth-token-exchange branch October 22, 2020 01:19
briandealwis added a commit to briandealwis/skaffold that referenced this pull request Nov 27, 2020
nkubala pushed a commit to GoogleContainerTools/skaffold that referenced this pull request Nov 30, 2020
…5064)

* Update pack v0.15.0 and drop replace of Azure/go-autorest

* go mod vendor

* Update code to pack v0.15 (debug still requires adjusting)

* Add debug support for CNB Platform API 0.4

* missed tests

* Fix fallout from google/go-containerregistry#791

* Make the TestDebug/buildpacks integration tests pass

Tests were failing as I wasn't rewriting the entrypoint, so that
Platform 0.4-style process executables (/cnb/process/web) continued
through and the debug transformers wouldn't recognize the executable.

- fix adjustCommandLine() and add tests
- fix testutil.isNil to handle function pointers
- narrow isCNBImage() to only allow entrypoint to be 1-element array
  and add tests
- go style nits (rename isCnbImage -> isCNBImage, godoc style)

* Mark TestDevPortForwardDeletePod to be skipped due to flakiness
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.

None yet

4 participants