-
Notifications
You must be signed in to change notification settings - Fork 1
Schema #3
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
Conversation
38f598a to
40e7c40
Compare
constructor/_schema.py
Outdated
| - `None` (default): Does not protect the `base` environment. | ||
| - `dict`: The `base` environment will be protected. An empty dict will generate a default message. Otherwise, the content of the dictionary will result in a custom message. |
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.
If I read this, I would think that I could write a literal None, which is not correct. I would focus the description on that this is the content of the frozen file without going into the details about what they may contain (or you have to maintain the documentation about the file format). You can just refer to the CEP for details.
Something like:
The content of the
frozenfile marker (see CEP-22 for details). If not used, thebaseenvironment will not be protected.
marcoesters
left a comment
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.
I think the code and documentation are in need of simplification. It also looks like the tests aren't passing.
constructor/_schema.py
Outdated
| message: NonEmptyStr | None = None | ||
| "Custom message to display when the environment is frozen." | ||
|
|
||
| error: NonEmptyStr | None = None |
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.
error is not a recognized field in a frozen file.
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.
Thanks for catching this! I thought removed all instances of error.
constructor/_schema.py
Outdated
| "Target channel, after being mapped" | ||
|
|
||
|
|
||
| class FreezeEnvConfig(BaseModel): |
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.
I don't think we should specify the fields here because this would commit us to keeping up with changes to the file specification, even if you allow for extra fields. I'd just use dict as the type and be done with it.
constructor/main.py
Outdated
| "conclusion_file", | ||
| "signing_certificate", | ||
| "post_install_pages", | ||
| "post_install_pages" |
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.
Please revert this. Trailing commas are standard.
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.
Typo from removing something else. Thanks!
constructor/main.py
Outdated
| and path.parts[0] == "envs" | ||
| and path.parts[-2:] == ("conda-meta", "frozen") | ||
| ) | ||
| # Frozen environments from freeze_base and freeze_env |
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.
This entire logic is getting very unwieldy. I think it's time to create a separate function (like validate_frozen) to perform all frozen environment validation.
constructor/main.py
Outdated
| frozens | ||
| and exe_type == StandaloneExe.CONDA | ||
| and check_version(exe_version, min_version="25.5.0", max_version="25.7.0") | ||
| and not check_version(exe_version, min_version="25.7.0") |
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.
This is incorrect. conda-standalone 25.5.x is the only version that can't handle this. The error text is misleading.
constructor/_schema.py
Outdated
| Use the standalone binary to perform the uninstallation on Windows. | ||
| Requires conda-standalone 24.11.0 or newer. | ||
| """ | ||
| freeze_base: dict[Literal["conda"], FreezeEnvConfig] | None = None |
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.
The documentation could be significantly condensed and doesn't specify the general case. The reason why we have the conda key is because we will, I think, eventually expand this to other package managers like pip. So, the first paragraph should consist of something like "Protects the conda environment against modifications. Supported formats are:", and then talk about the different supported package managers (which is just conda right now).
constructor/_schema.py
Outdated
| - An empty dictionary `{}` to create an empty frozen marker file and receive the default message | ||
| - A non-empty dictionary with desired content (i.e. message or other key-value pairs) to customize the frozen marker file | ||
| The dictionary content is written as-is to the frozen marker file. |
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.
This can be condensed to just say that the value is a dictionary that will be written into the frozen marker file. We don't have to explain the format to them - CEP-22 does that already.
constructor/_schema.py
Outdated
| The dictionary content is written as-is to the frozen marker file. | ||
| Alternatively, you can provide your own pre-created frozen marker file using the `extra_files` option. If both `freeze_base` and a custom frozen marker file listed under `extra_files` are provided for the same environment, the custom file will take precedence. |
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.
I think we can just write this as a warning that extra_files will overwrite the frozen file. If we raise a runtime error though, this paragraph can be removed entirely.
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.
I wouldn't remove it entirely even if we did raise an error, but I will shorten it a bit. I think it's nice for our users to make this note explicitly.
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 is the advantage of keeping it? The point of the metadata expansion is to provide a canonical way to protect environments. I don't think adding additional non-canonical ways to do the same task adds to clarity. We don't do the same for the condarc key for the same reason.
constructor/_schema.py
Outdated
| Alternatively, you can provide your own pre-created frozen marker file using the `extra_files` option. If both `freeze_base` and a custom frozen marker file listed under `extra_files` are provided for the same environment, the custom file will take precedence. | ||
| If none of the above are provided, the environment will not be protected. |
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.
I think that part is clear based on what that property does.
constructor/_schema.py
Outdated
| Example for default: | ||
| ```yaml | ||
| freeze_base: | ||
| conda: {} | ||
| ``` | ||
| Example for custom content: | ||
| ```yaml | ||
| freeze_base: | ||
| conda: | ||
| message: "This `base` environment is frozen and cannot be modified." | ||
| ``` |
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.
This is pretty much the same example. It just looks different because YAML doesn't have a different way to write an empty dictionary.
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.
Not quite, but yes, they do look very similar.
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.
The second example doesn't add significantly new information, especially since there isn't a "default frozen file". I think one example with the message is good enough for brevity.
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Co-authored-by: Marco Esters <mesters@anaconda.com>
Co-authored-by: Marco Esters <mesters@anaconda.com>
Co-authored-by: Marco Esters <mesters@anaconda.com>
Co-authored-by: Marco Esters <mesters@anaconda.com>
Co-authored-by: Marco Esters <mesters@anaconda.com>
Description
Provide metadata to easily protect environments (INST-622)
Related tickets:
frozen_environmentsmetadata (INST-635)Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?