Skip to content
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

Allow passing variables via raw YAML #354

Merged
merged 28 commits into from
Feb 29, 2024

Conversation

nkaretnikov
Copy link
Contributor

@nkaretnikov nkaretnikov commented Jan 8, 2024

Fixes #350.

Description

This pull request:

Tested manually:

  • variables is present and equal to {} when creating an environment
  • variables can be edited when creating an environment and the result can be later seen again via the new or admin UI
  • 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)
  • the server receives the provided variables value in action_solve_lockfile (in all of the above cases).

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • 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

Copy link
Contributor

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the urgency on this? Would it be too much to ask for some tests?

I'm just thinking that the repo already has a nice base of test files, and you already wrote up a set of manual tests, so maybe it wouldn't be too hard to turn some of those into automated tests. What do you think?

Copy link
Contributor

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested any of this, but I carefully reviewed the code. It looks great. I only spotted one thing, and I left an inline question.

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.

@trallard
Copy link
Collaborator

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

Copy link
Contributor

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nkaretnikov!

@nkaretnikov
Copy link
Contributor Author

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

@trallard trallard merged commit 6813f75 into conda-incubator:main Feb 29, 2024
4 checks passed
gabalafou pushed a commit to gabalafou/conda-store-ui that referenced this pull request Mar 5, 2024
* Allow passing `variables` via raw YAML

Fixes conda-incubator#350.

* Replace `formatCode` with `stringify`

* Use `variables` from the global state

* Do not repeat `variables` twice

* Get back to using `formatCode`

The custom pretty-printer is there to avoid printing `[]` when no data
is available, which is not very user-friendly.

* Rename `variables` to `environmentVariables`

* Check if text input is working in playwright tests

* Fixup: serialize as `variables`

* Add tests

* Add more tests

* Fixup: serialize as `variables` on create

* Update tests

* Fixup: serialize as `variables` on edit

* Fixup: serialize as `variables` on edit

* Rename back to `variables` in `CondaSpecification`

* Fixup: rename back to `variables` to avoid serialization bugs

* Fix the test

* Revert "Check if text input is working in playwright tests"

This reverts commit 5f08435.

* Update tests

* Create a new test

* Check siblings

* Add waitFor

* Run linter

* Save variables on toggle on create env

* Save variables on toggle on edit env

* Revert all test changes

* Remove redundant dispatch

* Fix a broken test
gabalafou pushed a commit to gabalafou/conda-store-ui that referenced this pull request Jul 9, 2024
* Allow passing `variables` via raw YAML

Fixes conda-incubator#350.

* Replace `formatCode` with `stringify`

* Use `variables` from the global state

* Do not repeat `variables` twice

* Get back to using `formatCode`

The custom pretty-printer is there to avoid printing `[]` when no data
is available, which is not very user-friendly.

* Rename `variables` to `environmentVariables`

* Check if text input is working in playwright tests

* Fixup: serialize as `variables`

* Add tests

* Add more tests

* Fixup: serialize as `variables` on create

* Update tests

* Fixup: serialize as `variables` on edit

* Fixup: serialize as `variables` on edit

* Rename back to `variables` in `CondaSpecification`

* Fixup: rename back to `variables` to avoid serialization bugs

* Fix the test

* Revert "Check if text input is working in playwright tests"

This reverts commit 5f08435.

* Update tests

* Create a new test

* Check siblings

* Add waitFor

* Run linter

* Save variables on toggle on create env

* Save variables on toggle on edit env

* Revert all test changes

* Remove redundant dispatch

* Fix a broken test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[BUG] - Add support for variables in the conda specification
3 participants