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.

Sign up for GitHub

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

Jump to bottom

Add subcommands to modify env vars using Job API #1944

Merged
merged 3 commits into from
Mar 15, 2023
Merged

Add subcommands to modify env vars using Job API #1944

merged 3 commits into from
Mar 15, 2023

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Feb 7, 2023 •
edited
Loading

This is the "client half" of #1943. It adds a client for the socket-based current-job API, and three new subcommands that exercise the Get, Update, and Delete methods:

# Get the current environment variables and values from the job executor
buildkite-agent env get
# Add or update one or more environment variable values in the job executor
buildkite-agent env set ...
# Un-set one or more environment variable values in the job executor
buildkite-agent env unset ...

Edit 1: renamed "delete" to "unset"

@DrJosh9000 DrJosh9000 force-pushed the env-subcommands branch 4 times, most recently from 378080a to 18e32fb Compare February 28, 2023 07:02
@DrJosh9000 DrJosh9000 added this to the v3.45 milestone Feb 28, 2023
@DrJosh9000 DrJosh9000 requested a review from moskyb March 1, 2023 04:14
@DrJosh9000 DrJosh9000 changed the title WIP: Add subcommands using current-job API Add subcommands to modify env vars using Job API Mar 1, 2023
@DrJosh9000 DrJosh9000 marked this pull request as ready for review March 1, 2023 04:14
@DrJosh9000 DrJosh9000 force-pushed the env-subcommands branch 8 times, most recently from a851f9c to 2628f58 Compare March 3, 2023 03:45
@pda
Copy link
Member

pda commented Mar 3, 2023

buildkite-agent env delete ...

I'd lean towards unset rather than delete, since you asked for opinions 😁
The former seems more unixy envish.

@DrJosh9000 DrJosh9000 force-pushed the env-subcommands branch 4 times, most recently from 0b57802 to d80e6eb Compare March 8, 2023 23:44
@DrJosh9000 DrJosh9000 force-pushed the env-subcommands branch 4 times, most recently from c55e724 to ca8dea2 Compare March 15, 2023 02:48
triarius
triarius reviewed Mar 15, 2023
View reviewed changes
clicommand/env_get.go Outdated Show resolved Hide resolved
clicommand/env_get.go Outdated Show resolved Hide resolved
clicommand/env_get.go Show resolved Hide resolved
clicommand/env_get.go Outdated Show resolved Hide resolved
clicommand/env_set.go Outdated Show resolved Hide resolved
clicommand/env_set.go Outdated Show resolved Hide resolved
clicommand/env_unset.go Outdated Show resolved Hide resolved
clicommand/env_unset.go Outdated Show resolved Hide resolved
jobapi/client.go
// do implements the common bits of an API call. req is serialised to JSON and
// passed as the request body if not nil. The method is called, with the token
// added in the Authorization header. The response is deserialised, either into
// the object passed into resp if the status is 200 OK, otherwise into an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

🥇

jobapi/client.go
dec := json.NewDecoder(hresp.Body)

if hresp.StatusCode != 200 {
var er ErrorResponse
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to put the status code in the ErrReponse? I guess it's YAGNI right now, and whoever needs it should be able to do it relatively easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, one for later.

triarius
triarius approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@triarius triarius left a comment

Choose a reason for hiding this comment

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

🚀 LGTM

@DrJosh9000 DrJosh9000 merged commit 79e285a into main Mar 15, 2023
@DrJosh9000 DrJosh9000 deleted the env-subcommands branch March 15, 2023 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@triarius triarius triarius approved these changes

@moskyb moskyb Awaiting requested review from moskyb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.45
Development

Successfully merging this pull request may close these issues.
3 participants
@DrJosh9000 @pda @triarius