Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding options to log addition additional metadata into the lockfile #204
Adding options to log addition additional metadata into the lockfile #204
Changes from 10 commits
d9b34a7
cfda7bd
604b9fe
f448f05
90875c3
f904e79
e7c613c
dd97f51
811662b
8f4c6af
fe54b6b
3820802
1fafc94
67cf13b
842702a
11dfd80
6f4b57a
e1cd6fa
7bb79ae
376b5d4
81a9e9c
d9ea458
0e3e51e
d91c42a
3f1ffc1
b346860
086d836
07f03f8
1afe3c8
f3f7170
cddab4e
5277b03
d356a05
4769460
a0bfe28
93a6f3e
9b7928c
27da398
8ce4b30
b1361d7
2917b0b
6897d20
8833927
7fafd4a
8cb4f44
2ef27bc
d003e43
3426fcf
c1e36ba
107508f
d871556
ef3cd98
f5761de
a833d13
4bee557
da86fac
ae256e3
941f0d8
1e910d6
645512d
19199f1
9a5292a
16e32c8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out-of-scope proposal:
I wanted to throw out this idea in case anyone else is interested in helping me tackle this idea in a subsequent PR... (I'm fully in favor of the current
created_at
implementation.)The disadvantage to
created_at
is that if you runconda-lock
again 1 minute later, then probably the only change in the lockfile will be to thecreated_at
field. Practically, knowing the exact minute that the lock was performed is superfluous, and it breaks idempotence.In order to restore idempotence, you could do (pseudocode)
and this
lock_time
represents the earliest possible instant when the lockfile could have theoretically been generated. Indeed, it depends only on the metadata of the locked packages themselves, so we could even consider enabling this by default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressing this comment, since the hash is already computed with
repo.head.object.hexsha
, I think it's just the description which needs to be fixed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to have the git metadata associated with the HEAD commit for the relevant repo or should it be associated with the most recent commit for the input files.
The latter would mean that if the components of a lock don't change then the git_meta is unaffected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if there were some way to include the SHA without also including my Git name/email.
I anticipate more metadata flags, so I'm wondering if we can make this more fine-grained and modular.
I have a similar but failed PR in which I implemented metadata selection via a single CSV argument like
--metadata=created_by,comment,command,timestamp
. I'm no design expert, so I'm not sure if this would be an improvement, but it may help with preventing a flood of options when runningconda-lock lock --help
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably having the metadata selector fields behave similar to how channels work. That jives with how the rest of the ecosystem functions. So something like
Fits most cleanly and works easily with click/typer which is what we're using for the cli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose I want to include something like
environment.yml
which contains lists. Shall we make this a fully general dict?(Needs
from typing import Any
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally not. It places a lot of extra strain on other implemations that make use of the file format like
micromamba
.Any
is FAR too broad a type. I can potentially see value in aUnion[str, list[str]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Ideally it would be some sort of recursive type like
T: Union[int, str, list[T], dict[str,T]]
, but Pydantic chokes on that already.Note how
requirements
must be at leastlist[Union[str, dict[str, list[str]]]]
due to["pip", {"pip": ["pypi-dep"]}]
. Thus to parse mostrequirements.txt
we'd need at least something like(assuming I didn't make any mistakes).
Given the complexity, I've convinced myself that this should be a separate feature. But in this case, we should probably make it clear in the
--help
that metadata-jsons/metadata-yamls contents must bedict[str,str]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to support
dict[str, Union[str, int, float]]
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm getting at here is, if we have
is it correct to have the current result of
conda-lock --metadata-yaml md.yaml
be as follows?Perhaps that's the simplest since strings faithfully encode most reasonable types, and it prevents other potential headaches.