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

[CIRCLE-24198] Add commands for managing contexts #362

Merged
merged 1 commit into from Feb 26, 2020

Conversation

@marcomorain
Copy link
Contributor

marcomorain commented Feb 7, 2020

Add a new command, context:

$ circleci context
Contexts provide a mechanism for securing and sharing environment variables across projects. The environment variables are defined as name/value pairs and are injected at runtime.

Usage:
  circleci context [command]

Available Commands:
  list        List contexts
  remove      Remove a secret from the named context
  show        Show a context
  store       Store an new secret in the named context. The value is read from stdin.

Currently, the commands enable users to list contexts, show contexts, and to add and
remove variables from contexts.

Not yet supported / things to do:

  • write tests
  • create context
  • delete context
  • CRUD on context groups
@marcomorain marcomorain requested a review from CircleCI-Public/x-team as a code owner Feb 7, 2020
@marcomorain marcomorain force-pushed the experimental-context-api branch 4 times, most recently from 02aa8f2 to 72cc302 Feb 7, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 10, 2020

Codecov Report

Merging #362 into master will increase coverage by 1.79%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
+ Coverage   28.55%   30.34%   +1.79%     
==========================================
  Files          23       25       +2     
  Lines        2760     3170     +410     
==========================================
+ Hits          788      962     +174     
- Misses       1884     2110     +226     
- Partials       88       98      +10     
Impacted Files Coverage Δ
api/context.go 45.92% <0.00%> (ø)
cmd/context.go 25.56% <0.00%> (ø)
api/api.go 3.93% <0.00%> (+2.84%) ⬆️

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 7f7e4e2...2e3457d. Read the comment docs.

@hannahhenderson hannahhenderson changed the title Add commands for managing contexts [CIRCLE-24198]Add commands for managing contexts Feb 12, 2020
Copy link
Contributor

aengelberg left a comment

Excited to see this new feature! I have a bunch of small ideas/concerns, but the naming of store/remove is probably the main one.

cmd/context.go Show resolved Hide resolved
git/git.go Outdated Show resolved Hide resolved
git/git.go Outdated Show resolved Hide resolved
git/git.go Outdated Show resolved Hide resolved
git/git.go Outdated Show resolved Hide resolved
git/git.go Outdated Show resolved Hide resolved
context.Node.CreatedAt,
})
}
table.Render()

This comment has been minimized.

Copy link
@aengelberg

aengelberg Feb 13, 2020

Contributor

Is this useful for a programmatic caller of the CLI? I feel like we need a "plain" flag that just prints out a list of names separated by newline or something.

This comment has been minimized.

Copy link
@marcomorain

marcomorain Feb 18, 2020

Author Contributor

Yes, I think that is valuable. I'll leave that for a second pass through this feature.

cmd/context.go Outdated Show resolved Hide resolved
@marcomorain marcomorain force-pushed the experimental-context-api branch 2 times, most recently from 73f92ca to 351f7b0 Feb 18, 2020
@marcomorain marcomorain changed the title [CIRCLE-24198]Add commands for managing contexts [CIRCLE-24198] Add commands for managing contexts Feb 18, 2020
@marcomorain marcomorain force-pushed the experimental-context-api branch 7 times, most recently from 9b4cedb to ad40c3d Feb 18, 2020
@marcomorain marcomorain requested a review from aengelberg Feb 20, 2020
@marcomorain marcomorain force-pushed the experimental-context-api branch from ad40c3d to 21361a6 Feb 20, 2020
Copy link
Contributor

glenjamin left a comment

Thoughts inline


func CreateContext(cl *client.Client, contextName, orgName, vcsType string) error {

contexts, err := ListContexts(cl, orgName, vcsType)

This comment has been minimized.

Copy link
@glenjamin

glenjamin Feb 21, 2020

Contributor

Is this just to get the org id? Is there an easier way?

This comment has been minimized.

Copy link
@marcomorain

marcomorain Feb 21, 2020

Author Contributor

That's a good point, this is a little sloppy to be pulling in the contexts when they are not strictly necessary. I can call api.getOrganization to request just the Org's ID.

Data: result,
}

server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {

This comment has been minimized.

Copy link
@glenjamin

glenjamin Feb 21, 2020

Contributor

Probably worth extracting a helper that takes the response and returns a client/server pair?

This comment has been minimized.

Copy link
@marcomorain

marcomorain Feb 21, 2020

Author Contributor

that's a good idea 👌

@@ -22,13 +22,13 @@ type Client struct {
Endpoint string
Host string
Token string
httpClient *http.Client
HttpClient *http.Client

This comment has been minimized.

Copy link
@glenjamin

glenjamin Feb 21, 2020

Contributor

You could use the address from the server rather making this public, but this seems fine too

}

command := &cobra.Command{
Use: "context",

This comment has been minimized.

Copy link
@glenjamin

glenjamin Feb 21, 2020

Contributor

Worth marking this hidden until we’ve done a few iterations so we can release it quietly early?

This comment has been minimized.

Copy link
@eric-hu

eric-hu Feb 24, 2020

Contributor

+1 for hiding this subcommand. Then this PR can be merged separately from context group CRUD


storeCommand := &cobra.Command{
Short: "Store an new secret in the named context. The value is read from stdin.",
Use: "store-secret <vcs-type> <org-name> <context-name> <secret name>",

This comment has been minimized.

Copy link
@glenjamin

glenjamin Feb 21, 2020

Contributor

Do we call them secrets elsewhere? I thought we normally called them values

This comment has been minimized.

Copy link
@marcomorain

marcomorain Feb 21, 2020

Author Contributor

🤔

image

We call them "Environment Variables" in the UI, and "Resources" in the GraphQL schema.

This comment has been minimized.

Copy link
@eric-hu

eric-hu Feb 24, 2020

Contributor

FWIW web-ui-org-settings renames them "Environment Variables" as well in its shim:

https://github.com/circleci/web-ui-org-settings/blob/5bcdce25669ba7fe8c5bfc91d4035f0f5199a49c/src/graphql/schema.graphql#L23


func contextByName(client *client.Client, vcsType, orgName, contextName string) (*api.CircleCIContext, error) {

contexts, err := api.ListContexts(client, orgName, vcsType)

This comment has been minimized.

Copy link
@glenjamin

glenjamin Feb 21, 2020

Contributor

Can the graphQL api filter for us, rather than having to download the whole list including env vars every time?

@marcomorain marcomorain force-pushed the experimental-context-api branch from 21361a6 to 1a7e425 Feb 21, 2020
}

func createContext(client *client.Client, vcsType, orgName, contextName string) error {
return api.CreateContext(client, vcsType, orgName, contextName)

This comment has been minimized.

Copy link
@eric-hu

eric-hu Feb 24, 2020

Contributor

I ran into an error when checking this locally:

 $ go run main.go context create github eric-hu foo
Error: Unable to find organization eric-hu of vcs-type foo: Provided argument value `foo' is not member of enum type.
exit status 255

It looks like api.CreateContext uses a different argument ordering:

https://github.com/CircleCI-Public/circleci-cli/pull/362/files#diff-9f8031b3385a5e9aa0cdd6f639ce2aa0R51

Copy link
Contributor

eric-hu left a comment

Ran through the new subcommands on this branch locally and found a small bug with context create. (Marek mentioned that Eco team has admin access to this repo now, so I'll leave it to y'all to formally approve, but this LGTM)

@marcomorain marcomorain force-pushed the experimental-context-api branch from 1a7e425 to 2cba6f0 Feb 24, 2020
@marcomorain marcomorain requested a review from CircleCI-Public/ecosystem Feb 24, 2020
@marcomorain marcomorain force-pushed the experimental-context-api branch from 2cba6f0 to 7285f28 Feb 25, 2020
@marcomorain marcomorain force-pushed the experimental-context-api branch 2 times, most recently from 858805c to 9c2de0d Feb 26, 2020
@marcomorain marcomorain force-pushed the experimental-context-api branch from 9c2de0d to f4c15ac Feb 26, 2020
Add a new command, `context`:

```
$ circleci context
Contexts provide a mechanism for securing and sharing environment variables across projects. The environment variables are defined as name/value pairs and are injected at runtime.

Usage:
  circleci context [command]

Available Commands:
  list        List contexts
  remove      Remove a secret from the named context
  show        Show a context
  store       Store an new secret in the named context. The value is read from stdin.
```

Currently, the commands enable users to list contexts, show contexts, and to add and
remove variables from contexts.

Not yet supported / things to do:

- write tests
- delete context
- CRUD on context groups

Co-Authored-By: Alex Engelberg <alex.engelberg@circleci.com>
@marcomorain marcomorain force-pushed the experimental-context-api branch from f4c15ac to 2e3457d Feb 26, 2020
@marcomorain marcomorain merged commit 9ebc42b into master Feb 26, 2020
3 checks passed
3 checks passed
ci Workflow: ci
Details
codecov/patch 37.46% of diff hit (target 28.55%)
Details
codecov/project 30.34% (+1.79%) compared to 7f7e4e2
Details
@marcomorain marcomorain deleted the experimental-context-api branch Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.