Allow passing `variables` via raw YAML

[nkaretnikov]

Fixes #350.

Description

This pull request:

• is a companion PR to https://github.com/conda-incubator/conda-store/pull/721
• allows passing variables via raw YAML, see: https://docs.conda.io/projects/conda/en/latest/user-guide/tasks/manage-virtual.html#overriding-detected-packages
• doesn't create new GUI elements (like the ones we have for channels and dependencies) because the conda-store server currently supports only CONDA_OVERRIDE_CUDA due to a conda-lock limitation (GUI elements can be added later once we have a chance to discuss how that should look like).

Tested manually:

• [x] variables is present and equal to {} when creating an environment
• [x] variables can be edited when creating an environment and the result can be later seen again via the new or admin UI
• [x] variables can be edited after the environment was created, the new result is saved and can be seen again via the new or admin UI (as part of the new build)
• [x] the server receives the provided variables value in action_solve_lockfile (in all of the above cases).

Pull request checklist

• [x] Did you test this change locally?
• [ ] Did you update the documentation (if required)?
• [x] Did you add/update relevant tests for this change (if required)?

Additional information

These tests should pass locally:

yarn run test test/environmentDetails/SpecificationEdit.test.tsx --coverage=false
yarn run test test/environmentCreate/SpecificationCreate.test.tsx --coverage=false

[trallard]

But I also have a more general question. Are we sure about the word choice for variables? I'm concerned that the name might be a little too generic, and we may come to regret the word choice later.

Agree with you here @gabalafou we should be favouring descriptive variable names, so I suggest something like conda_env_variable or the such @nkaretnikov

[nkaretnikov]

This is now blocked because CI is failing (unless this is urgently needed and we want to revert to my original version and merge without CI). The reason is explained in https://github.com/conda-incubator/conda-store-ui/pull/360#issuecomment-1923360060.

[nkaretnikov]

@trallard @gabalafou I renamed variables where possible and added new tests. PTAL.

About variable names: the choice of names matter or it can result in subtle bugs due to serialization. We want to make sure that we always generate variables when we serialize.

I ended up following the conventions of existing code: essentially, we need to use variables where dependencies is used and can use a more verbose name in places where requestedPackages is used. This is a good rule of thumb because it's not often clear that a certain object may be serialized later.

This is due to how JavaScript objects are serialized. Note how the key is created automatically here based on the variable name:

> var foo = 'bar'
undefined
> JSON.stringify({foo})
'{"foo":"bar"}'

[nkaretnikov]

The test code needs to be rewritten (there are many things wrong with the added tests, it will save me a lot of time if we could review them together synchronously)

Scheduled a meeting to discuss this.

[trallard]

Thanks for your review @gabalafou, as usual it is a critical and in depth one I agree with having the code and tests fixed before merging.

[nkaretnikov]

We had a call about tests in this PR. The comments above are from that call, but there's one issue we didn't have time to get to:

@gabalafou raised an issue about us comparing textContent to raw strings in the testsuite, because it might potentially break later when the rendering logic changes (such as when we update a dependency or refactor the code), since the strings include whitespace. Gabriel wanted to see the code that generates these on the UI side to better understand how this should be checked instead.

My answer to this is as follows: checking the raw output is the whole point of the test! From this standpoint, it's irrelevant how the current code is written. What I'm trying to avoid is potential future problems when someone updates the logic in the codebase or some dependency breaks (someone might add a new field or remove a field or change a list to a dict or something else). This format is what the users will see and it needs to match the conda format as well. Ideally, I'd like this tests to include newlines as well, but it's not possible when using textContent because newlines are rendered via CSS (the entries in the YAML text field are div and span elements with attributes).

As for these tests breaking because some dependency started to format lists with 4 spaces by default instead of 2, I'm not really worried about that. It'd be easy to update. However, I think there's real value in detecting formatting/generation errors early as opposed to checking other parts of the system (the formatting issue here might manifest as an input issue in conda or it might just be ignored, but the users will see strange output in the UI, which doesn't really match the conda spec). And yes, ideally, I'd include newlines as well, but it's tricky to do because of how this is rendered (see the comment about CSS above). So the current approach is the second best thing to getting 1-to-1 raw YAML output out of the system. I'll also add that this is how the current playwright tests are written as well, see test/playwright/test_ux.py:

page.get_by_text("channels: - conda-forgedependencies: - rich - pip: - nothing - ipykernel").fill("channels:\n  - conda-forge\ndependencies:\n  - rich\n  - python\n  - pip:\n      - nothing\n  - ipykernel\n\n")

That said, I'd be happy to reconsider and welcome suggestions! I'm just not sure how else we can write these to catch potential future changes to the generated YAML.

[nkaretnikov]

I had another meeting about this with @gabalafou. We discussed tests comparing against raw strings.

I think I'll just try to rewrite these tests such that we check: if <key>, then <value> == <expected> and try to do it in a way that ignores newlines/whitespace.

This will address the point that Gabriel made about this potentially breaking for unrelated reasons, while satisfying what I want to test for, which is that variables have very specific names.

I still think we should find a way to capture the entire output, newlines included, and test that as well, somewhere. But it's something we can postpone for later.

[nkaretnikov]

@gabalafou

Note: I still need to fix the issue where variables are not saved when you switch from/to YAML. I'll do so shortly.

But I've updated the tests based on our discussions. PTAL.

The reason I'm using textContent ... toBe is because it matches the string end-to-end. If you prefer strings not to have leading whitespace at all, another option is to use .toHaveTextContent.

Compared with before, these tests now print the HTML node value when it fails to match. They also look at a single component instead of searching through the entire page.

I'm also attaching a screenshot, which shows how the HTML looks like when the YAML file has dependencies. It explains why I'm using nextElementSibling and closest. If you can think of a better way to access this information, I welcome your feedback.

[nkaretnikov]

The commits I pushed yesterday add support for (temporary) saving the state of variables when switching between GUI/YAML, which is another issue that was highlighted as part of this review. I think this addresses all the feedback I've received so far.

[nkaretnikov]

Gabriel and I had a very productive discussion about this. To get this accepted, I need to:

• Remove all changes to tests because these will be flaky and we don't have good testing infrastructure for those types of tests
• Remove extraneous dispatch calls (only a couple of those actually matter for saving the state).

[nkaretnikov]

@gabalafou PTAL. I'll rebase before landing. Do not want to rebase right now to avoid making the diff hard to read.

[nkaretnikov]

@trallard this is ready, but it needs your approval before I can merge.