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

docs: Getting started sandbox #4045

Merged
merged 17 commits into from
Sep 11, 2024
Merged

docs: Getting started sandbox #4045

merged 17 commits into from
Sep 11, 2024

Conversation

Jayclifford345
Copy link
Contributor

This PR adds the metadata required to sandbox the getting started guide of tempo. This will allow users to run the tutorial in an online VM. To test you can use this developer version: https://killercoda.com/grafana-labs-chaos-testing/course/tempo/quick-start

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2024

CLA assistant check
All committers have signed the CLA.

@knylander-grafana knylander-grafana added the type/docs Improvements or additions to documentation label Sep 3, 2024
@knylander-grafana
Copy link
Contributor

@Jayclifford345 This is SO cool! I love having this done.

@Jayclifford345
Copy link
Contributor Author

Jayclifford345 commented Sep 4, 2024

Hahahha no worries @knylander-grafana! It was really fun, I am just working through a PR with @jdbaldry within the transformer and once we merge that I can put this green for your review :)

@Jayclifford345
Copy link
Contributor Author

Okay @knylander-grafana, we are ready for your review!

@knylander-grafana
Copy link
Contributor

knylander-grafana commented Sep 5, 2024

Okay @knylander-grafana, we are ready for your review!

@Jayclifford345 What is the best way for me to review this PR? Are there things I should test for the automated content updates or just check out and build locally?

@jdbaldry
Copy link
Member

jdbaldry commented Sep 5, 2024

Okay @knylander-grafana, we are ready for your review!

@Jayclifford345 What is the best way for me to review this PR? Are there things I should test for the automated content updates or just check out and build locally?

You primarily want to check that none of the INTERACTIVE directives have damaged the rendered documentation page. I think Jay will check on his side that the transformed tutorial looks good.

@jdbaldry
Copy link
Member

jdbaldry commented Sep 5, 2024

Also, it would be great to have a technical writer review any of the instructional copy even if it doesn't end up in the documentation page so that it conforms to our style and is clear for the users of the course.

@knylander-grafana
Copy link
Contributor

knylander-grafana commented Sep 9, 2024

I've resolved most of the issues in the PR. Per Jack's suggestion, I've asked for another writer to review the PR. I'll clear the request changes after the other PR review.

Copy link
Contributor

github-actions bot commented Sep 9, 2024

This PR must be merged before a backport PR will be created.

@jdbaldry
Copy link
Member

I've resolved most of the issues in the PR. Per Jack's suggestion, I've asked for another writer to review the PR. I'll clear the request changes after the other PR review.

Oh I didn't mean another technical writer necessarily but I do think peer review is a good way to share knowledge and practice :)

@Jayclifford345
Copy link
Contributor Author

Hi all, I have removed the image for the Interactive learning environment. I agree with Kim and I am now in an artistic crisis over my own banner design :D. So, I will drop a note to the design team to see if we can get something less intrusive created but still indicate these sandboxes are part of the larger interactive learning effort

@Jayclifford345
Copy link
Contributor Author

I think for now however we can go without the image. So happy for it to be merged when ready

@knylander-grafana
Copy link
Contributor

knylander-grafana commented Sep 10, 2024

Hi all, I have removed the image for the Interactive learning environment. I agree with Kim and I am now in an artistic crisis over my own banner design :D. So, I will drop a note to the design team to see if we can get something less intrusive created but still indicate these sandboxes are part of the larger interactive learning effort

I lke the idea of the banner. The current design is a bit overwhelming on the documentation page. Do you have banners on the other Killer coda tutorial doc pages within the documentation or only on the Killer Coda published sites?

Is it enough to use the text on the product documentation and maybe have the banner only on the Killer Coda site? Does makes more sense to brand it more in that context?

docs/sources/tempo/getting-started/docker-example.md Outdated Show resolved Hide resolved
Comment on lines 49 to 51


Provide feedback, report bugs, and raise issues in the [Grafana Killercoda repository](https://github.com/grafana/killercoda).
Copy link
Contributor

@clayton-cornell clayton-cornell Sep 10, 2024

Choose a reason for hiding this comment

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

Suggested change
Provide feedback, report bugs, and raise issues in the [Grafana Killercoda repository](https://github.com/grafana/killercoda).
Provide feedback, report bugs, and raise issues in the [Grafana Killercoda repository](https://github.com/grafana/killercoda).

Extra CR/LF

Alloy includes a banner image at this point. There's no banner here? EDIT: I just saw the discussion around this. Nevermind.

Also, the Provide Feedback part is new compared to the Alloy version. Should I include this over in the Alloy tutorials?

Copy link
Contributor

@knylander-grafana knylander-grafana Sep 10, 2024

Choose a reason for hiding this comment

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

Seems like a good idea to update your own content, Clayton. @Jayclifford345 What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, more than happy to have the banner removed and this section updated. I still think some form of icon or branding, later on, to differentiate it from a normal admonition makes users know this tutorial includes a sandbox from the initial glance.

All URLs are fully qualified so pre-processing isn't necessary.
Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

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

I think we're good now, Jay. What do you think? Please check Clayton's questions and the frontmatter. In my comments, I changed the relrefs to full URLs, so the preprocessing shouldn't be needed.

@Jayclifford345
Copy link
Contributor Author

Hi @knylander-grafana!
Looks great to me, happy to go ahead with the merge. Just give me a ping and I can get the sandbox merged into production on the Killercoda side as well and added to the automation pipeline.

Just FYI it looks like all of @clayton-cornell changes have been added as it looked already updated but apologies if I missed those.

Huge thanks all for reviewing and getting this in. Its a big win!

@knylander-grafana
Copy link
Contributor

Hi @knylander-grafana! Looks great to me, happy to go ahead with the merge. Just give me a ping and I can get the sandbox merged into production on the Killercoda side as well and added to the automation pipeline.

Just FYI it looks like all of @clayton-cornell changes have been added as it looked already updated but apologies if I missed those.

Huge thanks all for reviewing and getting this in. Its a big win!

This is awesome! Let's go ahead and merge this. When you get the Killercoda side ready, then we'll merge the backport.

@knylander-grafana knylander-grafana merged commit bca38a2 into main Sep 11, 2024
18 checks passed
@knylander-grafana knylander-grafana deleted the sandbox-getting-started branch September 11, 2024 16:05
github-actions bot pushed a commit that referenced this pull request Sep 11, 2024
Co-authored-by: Jack Baldry <[email protected]>
Co-authored-by: Kim Nylander <[email protected]>
Co-authored-by: Clayton Cornell <[email protected]>
(cherry picked from commit bca38a2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-v2.6 type/docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants