-
Notifications
You must be signed in to change notification settings - Fork 365
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
fix: better error message for invalid Helm Chart path during init #2839
fix: better error message for invalid Helm Chart path during init #2839
Conversation
✅ Deploy Preview for devspace-docs canceled.Built without sensitive environment variables
|
980293d
to
514e395
Compare
Fixes DSP-5 |
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.
Nice work!
Just a couple minor changes.
Can you take a look at the review feedback? |
@lizardruss Sorry about that, missed this in my inbox. Thanks for reviewing. I'll take a look soon! |
Made the changes. Ran my test again. It looks good still (it behaves the same way as before where the error message talks about it being a directory).
|
0c638c6
to
54e6524
Compare
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.
Apologies, but the linting and e2e tests failed, but the cause may be unrelated to your changes. I'm aiming to resolve those with this PR.
Once complete, can you rebase your branch against main so we can try again?
@mattwelke I think you can go ahead and rebase this branch. There are some test flakes that will need more investigation, but imo they should not hold up this PR. |
Signed-off-by: Matt Welke <[email protected]>
Signed-off-by: Matt Welke <[email protected]>
1baa23d
to
84cbb8b
Compare
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.
LGTM!
What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix
What does this pull request do? Which issues does it resolve?
Resolves #2838. See that issue for details.
New error message displayed when following the repro steps documented in the issue:
Note the "path is not a directory".
Please provide a short message that should be published in the DevSpace release notes
Fixed an issue where DevSpace's error message given a path to a file within a Chart directory instead of the Chart directory itself was unclear
What else do we need to know?
I didn't see any tests that called the function I changed, so I did not update or add any tests. But I did build the binary using the instructions in CONTRIBUTING.md and manually tested it. I used the built binary to produce the new command output above.
I followed instructions in CONTRIBUTING.md to run unit tests and integration tests. Integration tests did not pass, but I'm creating this PR to see if it was just my local env that prevented them from passing.