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

Update variable get to retrieve multiple variables #114

Merged
merged 1 commit into from
Mar 1, 2023
Merged

Update variable get to retrieve multiple variables #114

merged 1 commit into from
Mar 1, 2023

Conversation

rpothier
Copy link
Contributor

@rpothier rpothier commented Feb 24, 2023 •
edited
Loading

Desired Outcome

The variable get command should support getting multiple variables.

Implemented Changes

Updated the variable command to support multiple variables.
Updated go.mod to pull in the latest api that includes the latest hostfactory ID parser.

Getting a single variable will print out the variable as before

root@addd497790bd:/home/rob/cyberark/conjur-cli-go#  conjur variable get -i secret
George

Getting multiple secrets will print out the secret and the full variable name

root@0d7803c6039d:/home/rob/cyberark/conjur-cli-go# conjur variable get -i secret,top-secret
{
    "secret": "George"
    "top-secret": "Astro"
}

Note: The Python CLI requires the two variables to be separated by a space, and the conjur CLI
requires a comma

Connected Issue/Story

CyberArk internal issue ID: ONYX-33517

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a
    CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR
  • A follow-up issue to update official docs has been filed here: [insert issue ID]
  • This PR does not require updating any documentation

Behavior

  • This PR changes product behavior and has been reviewed by a PO, or
  • These changes are part of a larger initiative that will be reviewed later, or
  • No behavior was changed with this PR

Security

  • Security architect has reviewed the changes in this PR,
  • These changes are part of a larger initiative with a separate security review, or
  • There are no security aspects to these changes

@rpothier rpothier force-pushed the variable-get-multiple-vars branch 5 times, most recently from 169fe19 to 3b03216 Compare February 24, 2023 22:07
@rpothier rpothier marked this pull request as ready for review February 24, 2023 22:17
@rpothier rpothier requested a review from a team as a code owner February 24, 2023 22:17
@rpothier rpothier self-assigned this Feb 24, 2023
@szh szh self-requested a review February 27, 2023 13:52
szh
szh reviewed Feb 27, 2023
View reviewed changes
go.sum Show resolved Hide resolved
pkg/cmd/variable.go Show resolved Hide resolved
pkg/cmd/variable.go Outdated
if len(data) > 1 {
cmd.Println("{")
for k, v := range data {
cmd.Printf(" %s: %s\n", k, string(v))
Copy link
Contributor

Choose a reason for hiding this comment

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

This generates json output like this:

{
  dev:variable:secret: testing
  dev:variable:top-secret: testing
}

Is that the same as the Python CLI used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The python CLI quotes the variables and only displays the partial id

{
    "secret": "123",
    "top-secret": "456"
}

Updated to match the Python CLI

john-odonnell
john-odonnell suggested changes Feb 27, 2023
View reviewed changes
pkg/clients/clients.go Outdated Show resolved Hide resolved
@rpothier rpothier force-pushed the variable-get-multiple-vars branch 3 times, most recently from 606988a to b0a6ed1 Compare February 28, 2023 14:56
john-odonnell
john-odonnell suggested changes Mar 1, 2023
View reviewed changes
pkg/cmd/variable.go
if versionStr == "" {
data, err = client.RetrieveSecret(id)
data, err = client.RetrieveBatchSecretsSafe(id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if there's any detriment to using RetrieveBatchSecretsSafe for a single variable instead of using RetrieveSecret - did some experimenting with dev environment, and they seem to be equally performant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are similar APIs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the REST API for the two versions, there doesn't seem to be much overhead using the batch version.

curl -H "$(conjur authenticate -H)" http://conjur/secrets/dev/variable/secret
George
curl -H "$(conjur authenticate -H)" http://conjur/secrets?variable_ids=dev:variable:secret
{"dev:variable:secret":"George"}

pkg/cmd/variable.go Show resolved Hide resolved
pkg/cmd/variable.go Outdated
Long: `Use the get subcommand to get the value of one or more Conjur variables

Examples:
- conjur variable set -i secret -v my_secret_value
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: set example in get help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch, removed that one.

@rpothier rpothier force-pushed the variable-get-multiple-vars branch from b0a6ed1 to ab79170 Compare March 1, 2023 20:15
@codeclimate
Copy link

codeclimate bot commented Mar 1, 2023

Code Climate has analyzed commit ab79170 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 81.1% (0.2% change).

View more on Code Climate.

john-odonnell
john-odonnell approved these changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@john-odonnell john-odonnell left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@rpothier rpothier merged commit a0412b0 into master Mar 1, 2023
@rpothier rpothier deleted the variable-get-multiple-vars branch March 1, 2023 22:08
@john-odonnell john-odonnell mentioned this pull request Mar 3, 2023
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szh szh szh left review comments

@john-odonnell john-odonnell john-odonnell approved these changes

Assignees

@rpothier rpothier

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rpothier @szh @john-odonnell