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

search-replace function blockers #1821

Closed
frankfarzan opened this issue Apr 26, 2021 · 4 comments
Closed

search-replace function blockers #1821

frankfarzan opened this issue Apr 26, 2021 · 4 comments
Assignees
Labels
area/fn-catalog Functions Catalog size/S 1 day triaged Issue has been triaged by adding an `area/` label
Milestone

Comments

@frankfarzan
Copy link
Contributor

frankfarzan commented Apr 26, 2021

These docs do not work:

https://github.com/GoogleContainerTools/kpt-functions-catalog/tree/master/functions/go/search-replace

Multiple issues:

  1. This runs successfully without validating at least a by- flag is specified:
» kpt fn run --image gcr.io/kpt-fn/search-replace:unstable .
  1. In addition, where there are no matches, there's no indication just a blank line printed:
$ kpt fn run --image gcr.io/kpt-fn/search-replace:unstable . -- 'by-path=metadata.name' 'put-value=the-deployment'

  1. Also, the docs here incorrectly refer to apply-setters, probably a copy-paste issue.

  2. In general, findings are not printed today.

Please do an audit of the usage doc, make sure everything works as expected, corner cases are tested, etc.

@frankfarzan frankfarzan added area/fn-catalog Functions Catalog triaged Issue has been triaged by adding an `area/` label labels Apr 26, 2021
@frankfarzan frankfarzan added this to the v1.0 m2 milestone Apr 26, 2021
@frankfarzan frankfarzan added this to To do in kpt kanban board via automation Apr 26, 2021
@frankfarzan frankfarzan changed the title search-replace doesn't validate input search-replace doesn't validate input and doesn't show output Apr 26, 2021
@frankfarzan frankfarzan changed the title search-replace doesn't validate input and doesn't show output search-replace function doesn't work Apr 26, 2021
@phanimarupaka
Copy link
Contributor

phanimarupaka commented Apr 30, 2021

These docs do not work:

https://github.com/GoogleContainerTools/kpt-functions-catalog/tree/master/functions/go/search-replace

Multiple issues:

  1. This runs successfully without validating at least a by- flag is specified:

Ack. GoogleContainerTools/kpt-functions-catalog#264

» kpt fn run --image gcr.io/kpt-fn/search-replace:unstable .
  1. In addition, where there are no matches, there's no indication just a blank line printed:
$ kpt fn run --image gcr.io/kpt-fn/search-replace:unstable . -- 'by-path=metadata.name' 'put-value=the-deployment'

As in no output? This was intentional. Other tools like yq also does the same rather than throwing an error. The current behavior is correct IMO.

  1. Also, the docs here incorrectly refer to apply-setters, probably a copy-paste issue.
    Ack.
  2. In general, findings are not printed today.

This will be addressed by #1557

Please do an audit of the usage doc, make sure everything works as expected, corner cases are tested, etc.

@phanimarupaka
Copy link
Contributor

@frankfarzan 1,2,3 issues are resolved. 4 is a general issue with functions output in general and is being tracked here #1557, should we keep this till it is closed? And the issue description is not very accurate as replace works fine and search also produces results if --results-dir is specified. Issue is with displaying output. Consider changing the description.

@frankfarzan
Copy link
Contributor Author

There's a reference in the book to this issue. Let's leave this open even if the issue is not in the func impl as a signal to the readers of the book.

@frankfarzan frankfarzan changed the title search-replace function doesn't work search-replace function blockers May 4, 2021
@mikebz mikebz moved this from In Review to To do in kpt kanban board May 4, 2021
@mikebz mikebz moved this from To do to In progress in kpt kanban board May 11, 2021
@mikebz mikebz added the size/S 1 day label May 11, 2021
@phanimarupaka phanimarupaka assigned droot and unassigned phanimarupaka May 12, 2021
@droot
Copy link
Contributor

droot commented May 12, 2021

These should be addressed with #1961

kpt kanban board automation moved this from In progress to Done May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/fn-catalog Functions Catalog size/S 1 day triaged Issue has been triaged by adding an `area/` label
Projects
Development

No branches or pull requests

4 participants