-
Notifications
You must be signed in to change notification settings - Fork 176
Allow the user to specify individual credentials on the command line #531
Conversation
This is the basics, but I'm going to look at implementing support for |
I added a commit to allow I'm not 100% sure about this, especially the UX complexity needed to allow literal strings which start with |
Maybe the answer to "then we get into a complicated discussio" is a custom |
I am not sure about the "@" syntax. Something that might be used a lot (I think) is exec instead of reading a file (e.g. for executing a gcp/aws/azure CLI tool for obtaining a short lived token). This is already supported in credential-sets, and I suppose also with doing things like |
Exec instead of reading from a file is handled by the shell and not the tool I think (maybe it's different on windows?). I think the right thing to do is just drop the @ syntax, you can always use |
ff7cddd
to
b3daa6d
Compare
OK, dropped the final commit in favour of recommending the use of the shell syntax. I need to look at why the linter in complaining. |
b3daa6d
to
efcc62d
Compare
Linter fixed. |
There's a conflict, please rebase 🦁 |
Signed-off-by: Ian Campbell <ijc@docker.com>
efcc62d
to
c974239
Compare
Codecov Report
@@ Coverage Diff @@
## master #531 +/- ##
=========================================
Coverage ? 70.06%
=========================================
Files ? 55
Lines ? 3053
Branches ? 0
=========================================
Hits ? 2139
Misses ? 630
Partials ? 284
Continue to review full report at Codecov.
|
@silvin-lubecki rebase, dropped that blank line and fixed a random bug I spotted while refactoring during the rebase (see the new first commit) |
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.
LGTM 👍
Tests are failing
|
Urk, it passed on the other CI. It must be somehow non-deterministic (not sure how I missed that!). I guess there is a map in there somewhere. Will see what I can do! |
Hm, from running: e2e$ DOCKERAPP_BINARY=../bin/docker-app DOCKERCLI_BINARY=../bin/docker-linux go test -test.run TestCredentials/missing -test.v -test.count=100 . It seems to have a ~8% failure rate (I'd have guessed either 50% or 33%) I guess that's why I didn't see it. It's easy enough to fix -- I'll just make sure only one credential is missing. |
e.g. docker app install --credential name=somevalue bundle.json Credentials added with `--credential` always come after those added with `--credential-set` (irrespective of the order on the command line). A credential specified with `--credential` cannot override any previous credential, including those specified in a credential set. The test bnudle used is based on /~https://github.com/deislabs/example-bundles/blob/0e8af9a2f1270bd72045a515637a432e74743d5d/example-credentials/bundle.json But with `cnab/example-credentials:latest` → a digested ref (with the digest I pulled today) Signed-off-by: Ian Campbell <ijc@docker.com>
c974239
to
9ece4d7
Compare
Fixed, it survived the |
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.
LGTM
e.g.
Credentials added with
--credential
always come after those added with--credential-set
(irrespective of the order on the command line).A credential specified with
--credential
cannot override any previouscredential, including those specified in a credential set.
The test bnudle used is based on
/~https://github.com/deislabs/example-bundles/blob/0e8af9a2f1270bd72045a515637a432e74743d5d/example-credentials/bundle.json
But with
cnab/example-credentials:latest
→ a digested ref (with the digest Ipulled today)
Signed-off-by: Ian Campbell ijc@docker.com
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)