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

add troubleshooting section to variables doc #1417

Merged
merged 7 commits into from
Jan 28, 2025

Conversation

michelleN
Copy link
Member

Content must go through a pre-merge checklist.

Pre-Merge Content Checklist

This documentation has been checked to ensure that:

  • The title, template, and date are all set
  • Does this PR have a new menu item (anywhere in templates/*.hbs files) that points to a document .md that is set to publish in the future? If so please only publish the .md and .hbs changes in real-time (otherwise there will be a menu item pointing to a .md file that does not exist)
  • File does not use CRLF, but uses plain LF (hint: use cat -ve <filename> | grep $'\r' | wc -l and expect 0 as a result)
  • Has passed bart check
  • Has been manually tested by running in Spin/Bartholomew (hint: use PREVIEW_MODE=1 and run npm run styles to update styling)
  • Headings are using Title Case
  • Code blocks have the programming language set to properly highlight syntax and the proper copy directive
  • Have tested with npm run test and resolved all errors
  • Relates to an existing (potentially outdated) blog article? If so please add URL in blog to point to this content.

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

This is great! Thank you for writing it up! I left a couple of formatting nits but my main thought was to add a bit more remediation information. I've left suggestions although it might be better to treat them as inspiration rather than exact text! But if you're not keen to spend more time on this then no worries, we can merge as is and add more info later.

content/spin/v3/variables.md Outdated Show resolved Hide resolved
content/spin/v3/variables.md Outdated Show resolved Hide resolved

```console
Handler returned an error: Error::Undefined("no variable for \"<component-id>\".\"YOUR_VARIABLE\"")
```
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to add a "how to fix section" here e.g.

To fix this, edit spin.toml and add to the [component.<component-id>.variables] table a line such as <your-variable> = "{{ app-variable }}". See above for more information.

(sorry cannot figure out how to make GH show the above link as a literal Markdown link but hopefully it makes sense what I mean)

content/spin/v3/variables.md Outdated Show resolved Hide resolved
content/spin/v3/variables.md Show resolved Hide resolved
@michelleN
Copy link
Member Author

@itowlson I think I addressed all your feedback. Thank you. I also added some headings. Wdyt? I'm 50/50 on em. Thanks again.

@itowlson
Copy link
Contributor

Yeah they are pretty heavy eh. But the extra bit of structure is super useful, it really helps the eye to find the different errors/tips! How about if we made them just bold paragraphs instead of headings?

Another thought was to put them in terms of the errors people were getting rather than the causes/remedies (because someone with a woe doesn't know the cause/remedy yet, all the know is the error). What do you reckon?

Here is how those two changes (together) look on my machine:

image

Better? Worse?

michelleN and others added 7 commits January 28, 2025 13:12
Co-authored-by: itowlson <[email protected]>
Signed-off-by: Michelle Dhanani <[email protected]>
Co-authored-by: itowlson <[email protected]>
Signed-off-by: Michelle Dhanani <[email protected]>
Co-authored-by: itowlson <[email protected]>
Signed-off-by: Michelle Dhanani <[email protected]>
Signed-off-by: Michelle Dhanani <[email protected]>
@itowlson
Copy link
Contributor

@michelleN I found this languishing and I really think we should include it. I have rebased and made a minor tweak to the subheads. Hope that's okay. My changes are in a separate commit if you prefer to revert them!

@itowlson
Copy link
Contributor

Oh man I realised it was languishing because I had not sent an approval on it. I'm very sorry!

@michelleN michelleN merged commit e36b3e6 into fermyon:main Jan 28, 2025
3 checks passed
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.

2 participants