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

Support --input-type for exec-file #699

Merged
merged 3 commits into from Jul 28, 2020
Merged

Support --input-type for exec-file #699

merged 3 commits into from Jul 28, 2020

Conversation

lbonanomi
Copy link
Contributor

This PR resolves #652, adding support for an --input-type argument that will be honored by sops exec-file.

Until the argument for func outputStore was reversed (to use input-type instead of output-type) sops kept crashing with the message:

Error dumping file: No binary data found in tree

This is certainly a debatable fix, but results were positive.

@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2020

Codecov Report

Merging #699 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #699   +/-   ##
========================================
  Coverage    38.26%   38.26%           
========================================
  Files           23       23           
  Lines         3335     3335           
========================================
  Hits          1276     1276           
  Misses        1929     1929           
  Partials       130      130           

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 6b9e168...da41786. Read the comment docs.

cmd/sops/main.go Outdated
@@ -966,8 +970,9 @@ func inputStore(context *cli.Context, path string) common.Store {
return common.DefaultStoreForPathOrFormat(path, context.String("input-type"))
}

// SCARY! SHOULD THIS BE USING INPUT-TYPE?
Copy link
Contributor

Choose a reason for hiding this comment

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

No, using output-type is correct. Please revert me, or show why you think I'm wrong :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@autrilla I freely-admit that I'm probably poking the wrong part of this code. Here was my (simplistic) test.

With the file test containing only: "superSecret: superSecretValue"

and a sops/v3/cmd/sops/main.go containing:

func outputStore(context *cli.Context, path string) common.Store {
        //fmt.Println("Doing the right thing")
        //return common.DefaultStoreForPathOrFormat(path, context.String("output-type"))

        fmt.Println("Doing the wrong thing")
        return common.DefaultStoreForPathOrFormat(path, context.String("input-type"))
}

I get the following results:

# sops exec-file --input-type yaml /tmp/test 'cat {}'
Doing the right thing
Error dumping file: No binary data found in tree
# sops exec-file --input-type yaml /tmp/test 'cat {}'
Doing the wrong thing
superSecret: superSecretValue

Thank you for your time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see! I think the right thing to do here would be adding support for --output-type too. The issue is that SOPS tries to decide on the output store to use based on the file name, and assumes it should be binary (since /tmp/test does not have an extension that indicates a known file type). You'll need to make your calls to sops be of the form sops exec-file --input-type yaml --output-type yaml /tmp/test 'cat {}' afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the right thing to do here would be adding support for --output-type too.

Well that's fair. PR is updated for same.

Copy link
Contributor

@autrilla autrilla left a comment

Choose a reason for hiding this comment

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

LGTM other than the small change to the command line help

cmd/sops/main.go Outdated Show resolved Hide resolved
Co-authored-by: Adrian Utrilla <adrianutrilla@gmail.com>
@lbonanomi lbonanomi requested a review from autrilla July 27, 2020 23:50
@autrilla autrilla merged commit f78682c into getsops:develop Jul 28, 2020
@lbonanomi lbonanomi deleted the develop branch July 29, 2020 00:28
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

3 participants