atuin icon indicating copy to clipboard operation
atuin copied to clipboard

[feature] store env variables in History records

Open utterstep opened this issue 1 year ago • 4 comments

Whew! That's something...

Store env-variables (more precisely, configurable subset) which were set at the start of a command. Also closes #608, storing interpreter and allowing to filter by it.

Here is summary, broken down by commits. Also, I've left some comments here in this PR, hope it'll help to review.

  • 8e99b9b8f8ca179427fe344cef0ab111f8c9790e: remove HistoryWithoutDelete, add some docstrings, tests (now in #933)
  • 772429ef284d327a32a4bd1b9ef68b9c3df5c24a: (minor) clear apt cache in Dockerfile, to reduce resulting image size a bit (now in #931)
  • 0490de0da657a3f15e4914ea7406bb4c564605c4: create set of builders for History struct to simplify creation, while still having compile-time required fields, using typed-builder (now in #933)
  • 81ae5a2189946714086fa61c04be20c60ae38bb2: add interpreter field to History and allow to filter by it
  • a5f23aa5415441a58156676d7adccd663b3918ea: add context field to History, currently storing only environment variables, and add configuration to specify, which env vars to filter out
  • 44d897e8d64cd01c96e87a47237188bfffee8564: (minor) move contributors to separate text file in root (now in #932)
  • 96680fb73405422abe93191bb88edbe6f5cddaff: (minor) update History docs, listing available builders (now in #933)

PS Hope that large +diff would not scare you – I've also added bunch of docs and extracted CONTRIBUTORS in separate file, so not all of ~+900 is a pure code :)

utterstep avatar Apr 16 '23 09:04 utterstep

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
atuin ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 2, 2023 1:46pm

vercel[bot] avatar Apr 16 '23 09:04 vercel[bot]

Hi! I appreciate the PR, but this is far too much to review all at once. I respect that it's all split by commit, but I think it would be nice to have them split into multiple PRs.

Some preliminary feedback:

  • remove HistoryWithoutDelete - I know this is an awkward one, but we explicitly want it to be awkward for now so we can fix it properly - so I'd ask to not make this change.

The rest of the changes seem fine, although I'm not sure how necessary they all are to this feature change. I like the builder pattern and the Dockerfile change, but again they should really be separate PRs.

Also, while the interpreter and the env storage changes are similar in scope, and I appreciate you making them 2 different migrations to track, I would also appreciate separate PRs.

I know this seems very awkward, but managing an open source project with quite a number of users does become quite a maintenance challenge. We really appreciate the enthusiasm to make atuin a better tool, and we will eventually get this all merged, but we need to be extra careful. I hope you understand. Feel free to message us on discord if there's anything you want clarifying

conradludgate avatar May 03 '23 09:05 conradludgate

Hi, Conrad! No worries, I understand the difficulty and can split this PR in multiple ones.

Question about HistoryWithoutDelete – why do you need it? The solution I propose still has compatibility HistoryWithoutDelete would give (newer client + older data), and it still highlights the problem of incompatibility of older client with newer data. But it allows for adding fields without the need of HistoryWithoutDeleteAndInterpreterAndContext + HistoryWithoutInterpreterAndContext + HistoryWithoutContext in this case. Is that ok with you?

We can have this conversation in a different PR, if that would be more convenient for you.

And thanks in any case for your attention :)

utterstep avatar May 03 '23 10:05 utterstep

Created #931, #932 and #933 (updated PR summary to link which is which). I'll hold off interpreter and context/env_vars for now, as they are dependent on #933 :)

utterstep avatar May 03 '23 12:05 utterstep

Hi, @conradludgate, @ellie – what do you think about that? I created some separate PRs and can do something else required :)

Would like to continue work on that feature

utterstep avatar Jun 14 '23 17:06 utterstep

Hi, @conradludgate, @ellie – what do you think about that? I created some separate PRs and can do something else required :)

Would like to continue work on that feature

Hey! I'm sorry but I'm going to close this one for now. As a feature this would still be nice to have, however with the introduction of sync v2 and things like our key value store I think it's better suited to elsewhere. I'd also like to consider the ability to include the output of arbitrary shell commands as an attachment to history (so eg we could also store the current git branch, and this feature could just save env)

Apologies for the wasted time and energy here

ellie avatar Sep 11 '23 08:09 ellie

Ok, not a big deal, things happen :)

Let me know if you'll need any help someday

utterstep avatar Sep 11 '23 09:09 utterstep

@ellie What is the plan now for storing environment variables? I'd like to store the conda environment I'm in along with the commands so I can quickly filter based on that.

dufferzafar avatar Jul 12 '24 09:07 dufferzafar