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

refactor search and implement replace command #69

Merged
merged 5 commits into from
Feb 4, 2021
Merged

refactor search and implement replace command #69

merged 5 commits into from
Feb 4, 2021

Conversation

mattlqx
Copy link
Collaborator

@mattlqx mattlqx commented Feb 2, 2021

The replace command will do a match in the same fashion as grep but instead of presenting results with search patterns highlighted, it will highlight matches in red followed immediately by replacement string in green. In this way, it is easy to identify on one line what will be matched and replaced. by default, the operation will ask for confirmation before writing back to vault. it is possible to use flags -y to write without confirmation, -n to skip confirmation and not write aka dry-run, and limit scope to keys (-k) and values (-v) in the same way as in grep.

Moves search functions to its own file and provides a struct common for search/replace operations. The struct is embedded with commands that utilize search so that the commands contain the search paramters and the commands implement an interface for retrieving the parameters. This allows common methods to be located on a Searcher struct that can reference the command via the interface.

Screen Shot 2021-02-01 at 21 34 38

There's a lot of fat that can be trimmed from this implementation. I'm not super happy with it, but I think this is the kind of functionality I originally envisioned with #59 . I think in the future, it'd be nice to give the option of a traditional diff display instead of strictly inline color highlighting.

fishi0x01 and others added 3 commits February 1, 2021 21:40
moves search functions to its own file and provides a struct common for search/replace operations. the struct is embedded with commands that utilize search so that the commands contain the search paramters and the commands implement an interface for retrieving the parameters. this allows common methods to be located on a `Searcher` struct that can reference the command via the interface.
@mattlqx
Copy link
Collaborator Author

mattlqx commented Feb 2, 2021

I'll take a pass at codeclimate feedback tomorrow.

@mattlqx
Copy link
Collaborator Author

mattlqx commented Feb 3, 2021

Slightly refactored for codeclimate feedback. Not sure what to do with the test failing version check for dirty vs. not. I think this is in a good human review spot though. 👍🏻

@fishi0x01
Copy link
Owner

@mattlqx this looks great! Thank you 🙇

Concerning the failing test: For some reason, git describe --tags --always --dirty returns a different value in the test suite than in the Makefile. It should work when slightly adjusting Makefile and print-version.bats

It seems I cannot highlight/suggest changes here in files which were not changed.
Alternatively, I submitted a PR to your branch to highlight the diff - no need to merge that :)

@fishi0x01
Copy link
Owner

fishi0x01 commented Feb 3, 2021

@mattlqx it seems go.mod is not up-to-date. When I run make compile locally from the latest commit, then I see that go.mod is changed --> that change results in -dirty suffix.

That also explains why git describe --tags --always --dirty differs at compile and at test time.
Compile is run first, when the branch is clean, i.e., no -dirty flag. The compile changes go.mod file. Now integration test runs, and git describe --tags --always --dirty sees the change in go.mod file and adds -dirty suffix.

@mattlqx
Copy link
Collaborator Author

mattlqx commented Feb 3, 2021

Thanks, I'm still a noob on golang dependency stuff. A go get -u should've been enough to fix that, right?

@fishi0x01
Copy link
Owner

Afaik go get -u updates all dependencies - also the already locked ones.
make compile (go build) will update the go.mod file automatically if sth is missing - but it will not update existing deps.

I think the easiest way to resolve this, is to get the dep state from latest master again and run make compile.

One way this could be done:

  1. git checkout 59_replace
  2. rm -r vendor/ go.mod go.sum
  3. git checkout master
  4. cp -r vendor/ /tmp, cp go.mod /tmp, cp go.sum /tmp
  5. git checkout 59_replace
  6. mv /tmp/vendor/ .
  7. mv /tmp/go.mod .
  8. mv /tmp/go.sum .
  9. make compile
  10. git add vendor/

I will have a look at updating all dependencies on a different branch later. You dont need to fight through that :)

the `replace` command will do a match in the same fashion as `grep` but instead of presenting results with search patterns highlighted, it will highlight matches in red followed immediately by replacement string in green. in this way, it is easy to identify on one line what will be matched and replaced. by default, the operation will ask for confirmation before writing back to vault. it is possible to use flags `-y` to write without confirmation, `-n` to skip confirmation and not write aka dry-run, and limit scope to keys (`-k`) and values (`-v`) in the same way as in `grep`.
@mattlqx
Copy link
Collaborator Author

mattlqx commented Feb 3, 2021

Thanks. Looks like it's finally gotten past it. Maybe had something to do with my local go 1.16beta version on this Mac M1. I used a docker container of go 1.15.7 to build. Also just fyi, git checkout master go.sum go.mod vendor/ does the same thing as all those steps you listed. :)

@fishi0x01
Copy link
Owner

Wow git checkout is more powerful than I thought. Learned sth new today. Thx for the tip! It will save me a lot of time in the future :)

To be very honest the way I handle the deps here is also outdated afaik. You shouldn't take that as a positive example of how to do stuff in go :)
Nowadays most people dont include the vendor/ dir anymore, as it bloats up the repository. However, I actually like the static aspect of it. Further, it is already in the git history and for this project it doesn't bother me too much, so I keep it.

I will merge this PR later tonight and make a new release. Again, thank you @mattlqx for these awesome contributions. You have improved vsh a lot 🙇

@mattlqx
Copy link
Collaborator Author

mattlqx commented Feb 3, 2021

Thanks! This is going to be a very useful tool for managing our application configurations so I'm happy to help.

just want to make sure that the replace operation is not wiping out the rest of the secret.
@fishi0x01 fishi0x01 merged commit 75a69c9 into fishi0x01:master Feb 4, 2021
This was referenced Feb 4, 2021
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.

2 participants