Skip to content

Conversation

mikonieminen
Copy link

Add support for extracting sub-part of the document when running
exec-file or exec-env command.

@oneingan
Copy link

oneingan commented Aug 2, 2023

is there a chance to get this reviewed? is the only way to use a password file with a unique secret, like ansible requires.

@felixfontein
Copy link
Contributor

This looks good to me. @mikonieminen can you please rebase and sign-off the commit?

@felixfontein felixfontein added this to the v3.9.0 milestone Dec 17, 2023
@mikonieminen mikonieminen force-pushed the feature/extract-with-exec-file-and-exec-env branch from e41c965 to a644c4a Compare December 29, 2023 10:34
Add support for extracting subset of the document when running
exec-file or exec-env command.

Signed-off-by: Miko Nieminen <miko.nieminen@iki.fi>
@mikonieminen mikonieminen force-pushed the feature/extract-with-exec-file-and-exec-env branch from a644c4a to 49e56fe Compare December 29, 2023 10:46
@mikonieminen
Copy link
Author

This looks good to me. @mikonieminen can you please rebase and sign-off the commit?

Done and sorry for the delay. I hope it's good now.

What do you think about the error handling and messages? Maybe I should use toExitError(err) as in some other cases or do you think the current error handling is good?

Also, I've added example part in the command description. Do you think this is good or should that go somewhere else?

How about adding tests? I'm not experienced with go and not sure where and how to add these. Maybe in a separate PR since this has been pending quite some time and some other people have been asking for the functionality?

@felixfontein
Copy link
Contributor

Done and sorry for the delay. I hope it's good now.

Thanks, I think it looks good!

What do you think about the error handling and messages? Maybe I should use toExitError(err) as in some other cases or do you think the current error handling is good?

I think it is good this way.

Also, I've added example part in the command description. Do you think this is good or should that go somewhere else?

I think it's fine (also it has been there in the 'original' --extract).

How about adding tests? I'm not experienced with go and not sure where and how to add these. Maybe in a separate PR since this has been pending quite some time and some other people have been asking for the functionality?

I think tests would be great. I think functional tests would be suited best for exec-env and exec-file, but right now there are none so there's nothing you can easily copy. I'll try to create some for the existing exec-env and exec-file functionalities, then you can take a look at that PR.

@mikonieminen
Copy link
Author

How about adding tests? I'm not experienced with go and not sure where and how to add these. Maybe in a separate PR since this has been pending quite some time and some other people have been asking for the functionality?

I think tests would be great. I think functional tests would be suited best for exec-env and exec-file, but right now there are none so there's nothing you can easily copy. I'll try to create some for the existing exec-env and exec-file functionalities, then you can take a look at that PR.

That explains. Would be great, if you find the time to add tests that would be easy for me to use as an example for adding tests for these features. Happy to have a look at such PR.

Thanks.

@felixfontein
Copy link
Contributor

You can take a look at the latest commit of #1396. Right now I'm struggling with that the tests for the PR fail, while the same tests pass locally, and I have no clue why. This isn't the first time this is happening to me, and I'm still trying to figure out what happens...

@felixfontein
Copy link
Contributor

I finally managed to get #1396 working. I hope it'll get merged soon, then it should be easy to add a test for this :)

@felixfontein
Copy link
Contributor

#1396 got merged btw.

@felixfontein felixfontein modified the milestones: v3.9.0, 3.10.0 Jun 26, 2024
@felixfontein
Copy link
Contributor

@mikonieminen ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants