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 quick installation for maven to the registry #5330

Merged
merged 10 commits into from
Nov 11, 2024

Conversation

mercybassey
Copy link
Contributor

@mercybassey mercybassey commented Oct 5, 2024

This pull request fixes #5308 and adds quick installation for okHTTP Instrumentation

@mercybassey mercybassey requested a review from a team as a code owner October 5, 2024 16:50
@mercybassey
Copy link
Contributor Author

@svrnm am I supposed to make a change in quick install? I followed the same format in the entry.html however, it's showing compose require opentelemetry-instrumentation-okhttp instead of mvn install --. Additionally the the font awesome icon I added is not showing. Below is my output locally:

okhttp

I would appreciate your guidance on this. Thanks.

@mercybassey
Copy link
Contributor Author

@svrnm am I supposed to make a change in quick install? I followed the same format in the entry.html however, it's showing compose require opentelemetry-instrumentation-okhttp instead of mvn install --. Additionally the the font awesome icon I added is not showing. Below is my output locally:

okhttp

I would appreciate your guidance on this. Thanks.

Also the spelling check job is failing, did I do something wrong?

@svrnm svrnm changed the title Add more quick installations to the registry [outreachy] Add more quick installations to the registry Oct 7, 2024
@svrnm svrnm added the outreachy Issues for Outreachy Participants label Oct 7, 2024
@svrnm
Copy link
Member

svrnm commented Oct 7, 2024

hey @mercybassey, good start!

first of all, do not worry about the spell checker or any other issues, as long as the preview builds we are fine:

https://deploy-preview-5330--opentelemetry.netlify.app/

Let me take a look why it shows composer and not mvn

@svrnm
Copy link
Member

svrnm commented Oct 7, 2024

@mercybassey
Copy link
Contributor Author

https://deploy-preview-5330--opentelemetry.netlify.app/ecosystem/registry/?s=okhttp&component=&language=

works in the preview, maybe some local caching issue

@svrnm I ran npm run build again, and then npm run serve and It worked. Here's my output locally:
okhttp1

@mercybassey
Copy link
Contributor Author

@svrnm I noticed that the package information can be accessed using .<key> just like it was used in collector.md and hex.md and Hugo's split function uses a delimiter to separate strings and present them in the form of lists. Since the package name is io.opentelemetry.instrumentation/opentelemetry-okhttp-3.0 the spilt function converts it to ["io.opentelemetry.instrumentation", "opentelemetry-okhttp-3.0"] with \ as the delimiter.

Using Go's index function I was able to represent io.opentelemetry.instrumentation as the groupID and opentelemetry-okhttp-3.0 as the artifactID; and then reference it in entry.html. Below is my output locally:

okhttp2

I also removed installLine since it was doing something else.

@svrnm
Copy link
Member

svrnm commented Oct 7, 2024

great work 👍

@svrnm
Copy link
Member

svrnm commented Oct 10, 2024

lgtm overall, can you do the following please to fix the remaining CI issues:

  • update your branch with the latest commit to main
  • run npm run fix:all locally and commit the changes.

After you have done that, I would see this task as completed.

If you are open for an extra challenge, there is one more thing you can do: throughout the OpenTelemetry Java Documentation we provide instructions for Maven & Gradle, e.g. here is an example: https://opentelemetry.io/docs/languages/java/instrumentation/#dependency-management, and here you see how it is implemented using tabs: https://github.com/open-telemetry/opentelemetry.io/blob/main/content/en/docs/languages/java/instrumentation.md#dependency-management

So maybe you can update your could to support the maven snippet (that you have already) and the Gradle one, as listed here: https://central.sonatype.com/artifact/io.opentelemetry.instrumentation/opentelemetry-okhttp-3.0?smo=true by providing tabs as outlined above.

@svrnm
Copy link
Member

svrnm commented Oct 10, 2024

@mercybassey, apologies, but I just realized that you can not use shortcodes in that partial. This is a limitation of hugo I was running into multiple times already! This is not trivial to work around unfortunately, as you would need to replicate the whole tabbing logic. The easiest solution is just having gradle and maven below each other and stating that in a few sentences.

@mercybassey
Copy link
Contributor Author

Hi @svrnm I'm currently facing an issue when adding the tabs. I made the existing code snippet for Maven appear in a tab to see how it looks locally before adding the code snippets for Gradle. When I run npm run fix:all or npm run build I get the following error message:

Total in 33 ms Error: error copying static files: "/home/mercy/opentelemetry.io/layouts/partials/ecosystem/registry/quickinstall/maven.md:3:1": parse failed: template: partials/ecosystem/registry/quickinstall/maven.md:3: unexpected "%" in command

I noticed that opentelemetry.io uses the Docsy theme, so I followed the guide here. But I still cannot bypass or resolve the above error.

I'd appreciate your help on this. Thanks.

@mercybassey
Copy link
Contributor Author

@mercybassey, apologies, but I just realized that you can not use shortcodes in that partial. This is a limitation of hugo I was running into multiple times already! This is not trivial to work around unfortunately, as you would need to replicate the whole tabbing logic. The easiest solution is just having gradle and maven below each other and stating that in a few sentences.

Okay. I'll do just that.

@mercybassey
Copy link
Contributor Author

@mercybassey, apologies, but I just realized that you can not use shortcodes in that partial. This is a limitation of hugo I was running into multiple times already! This is not trivial to work around unfortunately, as you would need to replicate the whole tabbing logic. The easiest solution is just having gradle and maven below each other and stating that in a few sentences.

Okay. I'll do just that.

@svrnm I have added the code snippets for gradle, gradle(short) and gradle(kotlin). I'd like to know what you think. Thanks.

@mercybassey
Copy link
Contributor Author

I also ran npm run fix:all but some CI jobs are still failing.

@svrnm
Copy link
Member

svrnm commented Oct 10, 2024

I also ran npm run fix:all but some CI jobs are still failing.

it looks like maven does not like our crawler that we use to verify that their page is valid. That's out of scope for you to fix, let me do that for you.

I'd like to know what you think. Thanks.

It looks really great, thank you!

@svrnm
Copy link
Member

svrnm commented Oct 10, 2024

@mercybassey I added a fix for the CI issue, if you run git pull locally you should have that change as well.

@svrnm svrnm changed the title [outreachy] Add more quick installations to the registry ✅ [outreachy] Add more quick installations to the registry Oct 10, 2024
@svrnm
Copy link
Member

svrnm commented Oct 10, 2024

Thank you @mercybassey! This looks really good, I consider this as done! After the outreachy application phase we will take another look and see if we can merge this PR.

@mercybassey
Copy link
Contributor Author

Thank you @mercybassey! This looks really good, I consider this as done! After the outreachy application phase we will take another look and see if we can merge this PR.

Thank you.

@svrnm svrnm force-pushed the quick-installation-okhttp-java branch from 104a1a1 to d059939 Compare November 11, 2024 14:50
Signed-off-by: svrnm <[email protected]>
@svrnm svrnm changed the title ✅ [outreachy] Add more quick installations to the registry Add quick installation for mavent to the registry Nov 11, 2024
@svrnm svrnm added ux and removed outreachy Issues for Outreachy Participants labels Nov 11, 2024
@svrnm svrnm added this pull request to the merge queue Nov 11, 2024
@svrnm svrnm removed this pull request from the merge queue due to a manual request Nov 11, 2024
@svrnm svrnm changed the title Add quick installation for mavent to the registry Add quick installation for maven to the registry Nov 11, 2024
@svrnm svrnm added this pull request to the merge queue Nov 11, 2024
Merged via the queue into open-telemetry:main with commit 0ab4eb0 Nov 11, 2024
18 checks passed
@svrnm
Copy link
Member

svrnm commented Nov 11, 2024

@mercybassey thank you once again for your contribution!

drewby pushed a commit to drewby/opentelemetry.io that referenced this pull request Nov 21, 2024
drewby pushed a commit to drewby/opentelemetry.io that referenced this pull request Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[outreachy] Registry: add more quick installations to the registry
2 participants