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.

Sign up for GitHub

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

Jump to bottom

rad init validation blocks valid names (sadly) #5611

Closed
rynowak opened this issue May 29, 2023 · 0 comments · Fixed by #5614
Closed

rad init validation blocks valid names (sadly) #5611

rynowak opened this issue May 29, 2023 · 0 comments · Fixed by #5614
Labels
bug Something is broken or not working as expected

Comments

@rynowak
Copy link
Contributor

rynowak commented May 29, 2023 •
edited
Loading

Bug information

Steps to reproduce (required)

  1. Run rad init on a new cluster
  2. When prompted to enter an environment name try to type in prod-aws.

Observed behavior (required)

You will be stopped from entering the - and shown an error message.

image

Desired behavior (required)

The prompt for environment name allows me to enter prod-aws which is a valid name.

Workaround (optional)

I have not found a workaround, pasting doesn't work.

System information

rad Version (required)

edge (post v0.21

Additional context

See: charmbracelet/bubbles#185 and charmbracelet/bubbles#244

This is a bug in the bubbletea library that we're using for text input. The validation support blocks entering an invalid state on your way to a valid state.

Example:

Valid: prod-aws

Invalid: prod-

It's not legal for a resource name to have a trailing -, it has to end with an lowercase alphanumeric character. Since prod- is illegal, it gets blocked by bubbletea, which you means you can't enter the name you want.
@rynowak rynowak added the bug Something is broken or not working as expected label May 29, 2023
rynowak added a commit that referenced this issue May 29, 2023
@rynowak
Make validation tolerant of intermediate states
a221099 
Fixes: #5611

This change works around a bug in bubbletea's text component to give us the correct validation behavior we need. Unfortunately the text component's validation support does not allow intermediate states to be input.

For example if you're trying to input `prod-aws`, but `prod-` is invalid, then you'll just be stuck.

The fix for this is to handle validation ourselves at a higher level. This allows the text field to contain invalid values, but prevents them from being submitted.

I added some tests for the text component since we had none. Unfortunately I ran into the same bug as before with the teatest framework (truncated output). I was able to add basic test cases but blocked from doing anything advanced.
@rynowak rynowak mentioned this issue May 29, 2023
Merged
5 tasks
rynowak added a commit that referenced this issue May 30, 2023
@rynowak
Make validation tolerant of intermediate states
15cbffe 
Fixes: #5611

This change works around a bug in bubbletea's text component to give us the correct validation behavior we need. Unfortunately the text component's validation support does not allow intermediate states to be input.

For example if you're trying to input `prod-aws`, but `prod-` is invalid, then you'll just be stuck.

The fix for this is to handle validation ourselves at a higher level. This allows the text field to contain invalid values, but prevents them from being submitted.

I added some tests for the text component since we had none. Unfortunately I ran into the same bug as before with the teatest framework (truncated output). I was able to add basic test cases but blocked from doing anything advanced.
rynowak added a commit that referenced this issue May 31, 2023
@rynowak
Make validation tolerant of intermediate states (#5614)
61f99a3 
# Description

This change works around a bug in bubbletea's text component to give us
the correct validation behavior we need. Unfortunately the text
component's validation support does not allow intermediate states to be
input.

For example if you're trying to input `prod-aws`, but `prod-` is
invalid, then you'll just be stuck.

The fix for this is to handle validation ourselves at a higher level.
This allows the text field to contain invalid values, but prevents them
from being submitted.

I added some tests for the text component since we had none.
Unfortunately I ran into the same bug as before with the teatest
framework (truncated output). I was able to add basic test cases but
blocked from doing anything advanced.

## Issue reference

Fixes: #5611

## Checklist

Please make sure you've completed the relevant tasks for this PR, out of
the following list:

* [x] Code compiles correctly
* [x] Adds necessary unit tests for change
* [x] Adds necessary E2E tests for change
* [x] Unit tests passing
* [x] Extended the documentation / Created issue for it

## Auto-generated summary

<!--
GitHub Copilot for docs will auto-generate a summary of the PR
-->

<!--
copilot:all
-->
### <samp>🤖 Generated by Copilot at a221099</samp>

### Summary
🧪🐛🛠️

<!--
1. 🧪 - This emoji represents testing, experimentation, or science. It
can be used to indicate that the change adds new tests or improves the
test coverage of the codebase.
2. 🐛 - This emoji represents a bug, an error, or a flaw. It can be used
to indicate that the change fixes a bug or resolves an issue in the
codebase.
3. 🛠️ - This emoji represents a tool, a repair, or a construction. It
can be used to indicate that the change modifies or improves the
functionality or performance of the codebase.
-->
This pull request fixes a bug and adds tests for the `text` package in
the CLI prompt module. It replaces the `textinput` library validation
with a custom one and adds a new test file `text_test.go` with unit and
end-to-end tests. The tests use the `teatest` framework and cover some
common input scenarios.

> _Sing, O Muse, of the cunning code review_
> _That fixed the text input for the prompt,_
> _And added tests to check the model true_
> _With teatest framework and helpers prompt._

### Walkthrough
* Remove unused `errMsg` type from `text` package
([link](https://github.com/project-radius/radius/pull/5614/files?diff=unified&w=0#diff-9cc29225e4b4a9ce3eacd8d8c7f982efdf28c1a78fdb6c5f8fc84348adc82136L27-L30))
* Fix text input validation bug by bypassing library support and
implementing custom logic in `NewTextModel` and `Update` functions
([link](https://github.com/project-radius/radius/pull/5614/files?diff=unified&w=0#diff-9cc29225e4b4a9ce3eacd8d8c7f982efdf28c1a78fdb6c5f8fc84348adc82136L62-R71),
[link](https://github.com/project-radius/radius/pull/5614/files?diff=unified&w=0#diff-9cc29225e4b4a9ce3eacd8d8c7f982efdf28c1a78fdb6c5f8fc84348adc82136R95-R99),
[link](https://github.com/project-radius/radius/pull/5614/files?diff=unified&w=0#diff-9cc29225e4b4a9ce3eacd8d8c7f982efdf28c1a78fdb6c5f8fc84348adc82136L102-R117))
* Add unit and end-to-end tests for text input prompt in `text_test.go`
file, using `teatest` framework and helper functions
([link](https://github.com/project-radius/radius/pull/5614/files?diff=unified&w=0#diff-a95b3c117c873821d9473cd582ee12f2ffccb36c71d5fd41b1a3897f0268d957R1-R160))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something is broken or not working as expected
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make validation tolerant of intermediate states radius-project/radius
1 participant
@rynowak