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: fix installation steps in E2E example #1645

Merged
merged 1 commit into from
Jun 29, 2023
Merged

docs: fix installation steps in E2E example #1645

merged 1 commit into from
Jun 29, 2023

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Jun 28, 2023

Fixes: #1640

Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT odubajDT requested review from a team as code owners June 28, 2023 10:50
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jun 28, 2023
@netlify
Copy link

netlify bot commented Jun 28, 2023

Deploy Preview for keptn-lifecycle-toolkit ready!

Name Link
🔨 Latest commit f68857e
🔍 Latest deploy log https://app.netlify.com/sites/keptn-lifecycle-toolkit/deploys/649c1083815ce40008f7611d
😎 Deploy Preview https://deploy-preview-1645--keptn-lifecycle-toolkit.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sonarcloud
Copy link

sonarcloud bot commented Jun 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@mowies mowies changed the title docs: fix instalation steps in E2E example docs: fix installation steps in E2E example Jun 28, 2023
Copy link
Contributor

@StackScribe StackScribe left a comment

Choose a reason for hiding this comment

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

Could you explain a bit more about what the issue is here? Are the scripts other than make install broken?

I confess that I am troubled by this Getting Started guide. Having the scripts that set everything up is handy for a demo but seem inadequate for a Getting Started Guide since the user does not learn how to do the set up. This is, of course, outside the scope of this PR, but just saying ;-)

@agardnerIT
Copy link
Contributor

agardnerIT commented Jun 28, 2023

This PR looks good. None of the following comment should be seen as a blocker for getting this PR merged. But I agree with @StackScribe. A magic make command really doesn't help a new user understand anything about what's going on. We may as well provide a killercoda, pre-installed VM.

On a different note, there's this comment: "Note To export traces to the OpenTelemetry Collector, you need a KeptnConfig CRD with spec.OTelCollectorUrl specified in the namespace where KLT is installed."

The comment is ambiguous though as to whether or not the make install command will do that for us in the demo or we're left to add that in if we require it.

Since the emission of OTEL metrics is a core use case, should we build OTEL emission and a collector into the demo?

@odubajDT
Copy link
Contributor Author

Hey @agardnerIT @StackScribe . Thank you for your comments, totally agree! We should maybe work on a more specific explanation what is actually hidden behind make install. The aim of this PR was just to fix a small bug in the documentation (#1640). The previous version actually didn't work, as it did not install the KLT as part of make install-observability, so the excercise didn't work fully. We can maybe follow-up with a specific explanation in another PR? WDYT?

Thanks!

@odubajDT odubajDT merged commit d6f4307 into main Jun 29, 2023
19 checks passed
@odubajDT odubajDT deleted the fix/1640/demo branch June 29, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KLT End-to-end exercise broken
4 participants