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: creating a devworkspace telemetry plug-in #2231

Merged
merged 9 commits into from
Mar 23, 2022

Conversation

dkwon17
Copy link
Contributor

@dkwon17 dkwon17 commented Mar 10, 2022

Signed-off-by: David Kwon [email protected]

What does this pull request change

Adds a Creating a Telemetry plugin For DevWorkspaces section:

image

The new content is similar to the Creating a Telemetry plugin section, except that the new content is specific to DevWorkspaces.

This pull request also adds more details to the Telemetry section:
image

What issues does this pull request fix or reference

eclipse-che/che#20820
eclipse-che/che#20090

Specify the version of the product this pull request applies to

This pull request adds content to the 7.43.x version docs, but this pull request applies to any version that supports devworkspace telemetry, which I believe, should be version 7.27.x and above.

After the PR feedback and after this PR is merged, I would like to make a new PR to add the content of this PR to main, since this content is DevWorkspace-specific.

Pull request checklist

The author and the reviewers validate the content of this pull request with the following checklist, in addition to the automated tests.

  • Any procedure:
    • Successfully tested.
  • Any page or link rename:
  • Builds on Eclipse Che hosted by Red Hat.
  • the Validate language on files added or modified step reports no vale warnings.

@github-actions
Copy link

Click here to review and test in web IDE: Contribute

Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

left a couple of comments

@ibuziuk
Copy link
Member

ibuziuk commented Mar 15, 2022

@dkwon17 looks like there is one language suggestion that need to be addressed -
Use 'plug-in' rather than 'plugin'.

@dkwon17
Copy link
Contributor Author

dkwon17 commented Mar 15, 2022

There is a validation error in the CI here:
image

This error is referencing this part of the PR:

pass:[<!-- vale RedHat.TermsErrors = NO -->]
. Maven Quarkus project scaffolding:
+
----
$ mvn io.quarkus:quarkus-maven-plugin:2.7.1.Final:create \
-DprojectGroupId=mygroup -DprojectArtifactId=devworkspace-telemetry-example-plugin \
-DprojectVersion=1.0.0-SNAPSHOT
----
pass:[<!-- vale RedHat.TermsErrors = YES -->]

Although, I'm not sure how to fix this, since the code is already surrounded by pass:[<!-- vale RedHat.TermsErrors = NO -->] and pass:[<!-- vale RedHat.TermsErrors = YES -->].

Do you have any suggestions @rkratky, @themr0c ?

. Maven Quarkus project scaffolding:
+
----
$ mvn io.quarkus:quarkus-maven-plugin:2.7.1.Final:create \
Copy link
Contributor

Choose a reason for hiding this comment

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

@themr0c, is there a way to pass through the contents of a code block so that Vale does not throw an error?
image

Copy link
Contributor

@nickboldt nickboldt Mar 22, 2022

Choose a reason for hiding this comment

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

you could move the code to a separate file and include it, like on line 43 above?

include::example$creating-a-telemetry-plug-in-for-devworkspaces/main.go[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you could move the code to a separate file and include it, like on line 43 above?

I did this in my latest commit and there's no error on line 74 anymore. Still not completely sure why pass:[<!-- vale RedHat.TermsErrors = NO -->] and pass:[<!-- vale RedHat.TermsErrors = YES -->]. didn't suppress the error before.

I've added the passes in the latter part of the documentation and it is working there.

@ibuziuk
Copy link
Member

ibuziuk commented Mar 17, 2022

@themr0c please advise how can we set up the excludes for mvn command

@ibuziuk
Copy link
Member

ibuziuk commented Mar 22, 2022

@dkwon17 could you please resolve the conflicts ?

@nickboldt
Copy link
Contributor

nickboldt commented Mar 22, 2022

@dkwon17 could you please resolve the conflicts ?

Please confirm this is a good conflict fix. I couldn't find an existing doc for customizing workspaces components but did find https://www.eclipse.org/che/docs/che-7/end-user-guide/authoring-devfiles-version-2/ so I have assume @dkwon17 's link is better than what's in master branch sources.

4d2233b

Signed-off-by: David Kwon <[email protected]>
Signed-off-by: David Kwon <[email protected]>
Signed-off-by: David Kwon <[email protected]>
@ibuziuk
Copy link
Member

ibuziuk commented Mar 23, 2022

@dkwon17 looks great, added a single formatting update request (no new line for .sh)
@nickboldt @themr0c @max-cx folks, if you do not have any other suggestions, please approve

@nickboldt nickboldt self-requested a review March 23, 2022 11:20
@@ -1,6 +1,13 @@
[id="creating-a-telemetry-plugin_{context}"]
= Creating A Telemetry Plug-in

[WARNING]
====
This page explains how to create a back-end telemetry plug-in for {prod-short} workspaces, which will be deprecated in favour of {devworkspace}s.
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't write about the future in the docs.

Will removing this first sentence of this warning admonition make a difference to the meaning of the second sentence?

Suggested change
This page explains how to create a back-end telemetry plug-in for {prod-short} workspaces, which will be deprecated in favour of {devworkspace}s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, could this sentence instead be shortened to:

"This page explains how to create a back-end telemetry plug-in for {prod-short} workspaces."

instead?

How does that sound @max-cx @ibuziuk ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it would look like:
image

Copy link
Member

@ibuziuk ibuziuk Mar 23, 2022

Choose a reason for hiding this comment

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

looks good ^
In general the plan is to remove non-devworkspace part in the future PRs completely as part of eclipse-che/che#21264 cleanup

@dkwon17
Copy link
Contributor Author

dkwon17 commented Mar 23, 2022

Thank you for the reviews everyone, I have committed the suggestions.

@nickboldt nickboldt merged commit 24cf06c into eclipse-che:master Mar 23, 2022
@themr0c themr0c added this to the 7.44 milestone Apr 1, 2022
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.

6 participants