atuin
atuin copied to clipboard
[feature] store env variables in History records
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, usingtyped-builder
(now in #933) - 81ae5a2189946714086fa61c04be20c60ae38bb2: add
interpreter
field toHistory
and allow to filter by it - a5f23aa5415441a58156676d7adccd663b3918ea: add
context
field toHistory
, 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 :)
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 |
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
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 :)
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 :)
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
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
Ok, not a big deal, things happen :)
Let me know if you'll need any help someday
@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.