Skip to content

fix(deploy): use absolute path to load component source#593

Merged
itowlson merged 1 commit into
spinframework:mainfrom
FrankYang0529:fix-deploy-relative-path-error
Jun 26, 2022
Merged

fix(deploy): use absolute path to load component source#593
itowlson merged 1 commit into
spinframework:mainfrom
FrankYang0529:fix-deploy-relative-path-error

Conversation

@FrankYang0529

Copy link
Copy Markdown
Contributor

fixes #590

@radu-matei radu-matei requested a review from michelleN June 18, 2022 18:03
@FrankYang0529 FrankYang0529 force-pushed the fix-deploy-relative-path-error branch from f38598d to c273194 Compare June 20, 2022 12:48

@michelleN michelleN left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you @FrankYang0529 for fixing this bug. Manually tested. Also was nice to see the push method on PathBuf being used. I usually use join on Path so I got to learn something new. 🎉

We might also want to absolutize the app dir reference as a follow up.

Would you mind also signing your commit so we can merge?

@michelleN

michelleN commented Jun 20, 2022

Copy link
Copy Markdown
Collaborator

@FrankYang0529 info on signing if you need it: https://spin.fermyon.dev/contributing/

Lmk if you need any help.

@FrankYang0529 FrankYang0529 force-pushed the fix-deploy-relative-path-error branch from c273194 to fa9ff74 Compare June 20, 2022 14:29
@FrankYang0529

Copy link
Copy Markdown
Contributor Author

We might also want to absolutize the app dir reference as a follow up.

Do you mean also put app_folder in DeployCommand? If yes, I can help to do the follow up.

Would you mind also signing your commit so we can merge?

I sign it. Thank you.

@itowlson itowlson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks good - thanks! Comments are mostly minor nits; the only important thing is that when we try to open a file it's really important to provide a with_context with the file name because the stdlib error gives you no clue at all which file the operation has failed on - this was the issue I hit in #590! Thanks again!

Comment thread src/commands/deploy.rs Outdated
Comment thread src/commands/deploy.rs Outdated
Comment thread src/commands/deploy.rs Outdated
Comment thread src/commands/deploy.rs Outdated
Signed-off-by: Frank Yang <yangpoan@gmail.com>
@FrankYang0529 FrankYang0529 force-pushed the fix-deploy-relative-path-error branch from fa9ff74 to ac8ccd5 Compare June 21, 2022 10:27

@itowlson itowlson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM - thanks for the fixes!

@itowlson itowlson merged commit 87f74e7 into spinframework:main Jun 26, 2022
@FrankYang0529 FrankYang0529 deleted the fix-deploy-relative-path-error branch June 27, 2022 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deploy: uninformative error if run from another directory

3 participants