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

feat: Provides 'Adding a Visual Studio Code Extension' module #2265

Merged
merged 12 commits into from
Apr 7, 2022

Conversation

benoitf
Copy link
Contributor

@benoitf benoitf commented Mar 30, 2022

What does this pull request change

Introduce Adding a Visual Studio Code Extension module

What issues does this pull request fix or reference

eclipse-che/che#21296

Specify the version of the product this pull request applies to

master

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.

Change-Id: Ide2764fe5bb63fcd5d7592d5c75d67622a7b1cac
Signed-off-by: Florent Benoit fbenoit@redhat.com

reviewer: @deerskindoll

Change-Id: Ide2764fe5bb63fcd5d7592d5c75d67622a7b1cac
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
@benoitf benoitf requested review from rkratky and themr0c as code owners March 30, 2022 06:56
@github-actions
Copy link

Click here to review and test in web IDE: Contribute

@benoitf
Copy link
Contributor Author

benoitf commented Apr 6, 2022

@deerskindoll it seems that after your changes, the Pull Request is not passing anymore the vale checks

@deerskindoll
Copy link
Contributor

requesting peer review


{prod-short} has a default plug-in and extensions registry installed in every {prod-short} instance. The Che-Theia IDE searches and installs the plug-ins and extensions from that location.

Check this {prod-short} https://github.com/eclipse-che/che-plugin-registry[registry project] for an overview of default plug-ins, extensions, and source code. An online instance that is refreshed after every commit to the main branch, is located https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/[here]. You can use a different plug-in or extension registry for Che-Theia if you don't work in air gap mode: only the default registry is available there.
Copy link
Contributor

Choose a reason for hiding this comment

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

In some che-docs topics, link: precedes URLs like this, but not in this PR. Is that okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Check this {prod-short} https://github.com/eclipse-che/che-plugin-registry[registry project] for an overview of default plug-ins, extensions, and source code. An online instance that is refreshed after every commit to the main branch, is located https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/[here]. You can use a different plug-in or extension registry for Che-Theia if you don't work in air gap mode: only the default registry is available there.
Check this {prod-short} https://github.com/eclipse-che/che-plugin-registry[registry project] for an overview of [the?] default plug-ins, extensions, and source code. An online instance that refreshes after every commit to the main branch is located https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/[here]. You can use a different plug-in or extension registry for Che-Theia if you don't work in air gap mode: only the default registry is available there.

Copy link
Contributor

@rolfedh rolfedh Apr 6, 2022

Choose a reason for hiding this comment

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

I'm having trouble making sense of the final sentence.

  • I don't see an "air gap mode" mentioned in the che-docs. Would it help to say "air-gapped environment", link to a topic, or specify the key parameters for setting up an air-gapped environment, such as airGapContainerRegistryHostname?
  • @benoitf, does "only the default registry is available there" mean "only the default registry is available in an air-gapped environment?" This seems like the opposite of what I would expect. I expect the user would need to specify a custom (non-default) plug-in or extension registry because they are in an air-gapped environment. The default URLs of the plug-ins and extensions might not be reachable from an air-gapped system.

Copy link
Contributor

@rolfedh rolfedh Apr 6, 2022

Choose a reason for hiding this comment

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

Using the definite article, "the default plug-ins", makes more sense if the "registry project" topic describes specific plug-ins. Otherwise, if it's an abstract discussion of "plug-ins, extensions, and source code," the current wording is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'air gap' is also named 'restricted environment' sometimes. But you can find definition there https://en.wikipedia.org/wiki/Air_gap_(networking)

And it's used in Che (and docs)

Copy link
Contributor

@rolfedh rolfedh Apr 7, 2022

Choose a reason for hiding this comment

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

'air gap' is also named 'restricted environment' sometimes. But you can find definition there https://en.wikipedia.org/wiki/Air_gap_(networking)

And it's used in Che (and docs)

Although we should strive to use the terminology consistently, I'm not highlighting "air gap" as a problem. I'm concerned that calling it a "mode" might prompt users to search for an actual software mode when it seems like there might not be one. I'm guessing you mean "an air-gapped or restricted environment" instead.

Otherwise, if there is something like a mode, it's not called a "mode" in the software or documentation and actually consists of one or more parameters that the user must set when operating the software in a restricted environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll accept any suggested changes but here suggested changes have questions so I never saw this kind of suggested changes.

Does "only the default registry is available there" mean "only the default registry is available in an air-gapped environment?" This seems like the inverse of what I would expect: That the user probably requires use of a custom plug-in or extension registry because they are in an air-gapped environment. The default URLs of the plug-ins and extensions might not be reachable from an air-gapped system.

There is no need to use a custom plug-in or extension registry. Che deploy a registry on the air-gapped environment and in that case it includes all the binaries so there is no need to fetch something externally.

The default URLs of the plug-ins and extensions might not be reachable from an air-gapped system.

everything is reachable in that case (we build and deploy an offline / air-gapped environment registry)

Copy link
Contributor

Choose a reason for hiding this comment

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

wording fixed


The easiest way to add a Visual Studio Code extension to a workspace is to add it to the `.vscode/extensions.json` file. The main advantage of this method is that it works with all supported {prod-short} IDEs.

If you use the Che-Theia IDE, the extension is installed and configured automatically. If you use a different supported IDE with the Che-Code Visual Studio Code fork, a pop-up with a recommendation to install the extension shows when you start the workspace.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you use the Che-Theia IDE, the extension is installed and configured automatically. If you use a different supported IDE with the Che-Code Visual Studio Code fork, a pop-up with a recommendation to install the extension shows when you start the workspace.
If you use the Che-Theia IDE, the extension is installed and configured automatically. If you use a different supported IDE with the Che-Code Visual Studio Code fork, the IDE [displays a pop-up with a recommendation/promts you] to install the extension when you start the workspace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GitHub suggestion implies that we can commit directly the change.
Here it looks that there are two suggestions so it should be two separate GitHub PR suggestions (and people can approve one of them)

Copy link
Contributor

Choose a reason for hiding this comment

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

wording fixed

}
----

*NOTE*
Copy link
Contributor

Choose a reason for hiding this comment

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

For comparison, I searched the che-docs repo for admonitions like one this but didn't find any. You can use the asciidoc formatting for admonitions, which looks like this:

NOTE: adlfjakdfj

or

[NOTE]
====
afdasdf
====

The openshift-docs repo standardized on this later format...
I would recommend asking your team which one to use.

Copy link
Contributor

@rolfedh rolfedh Apr 7, 2022

Choose a reason for hiding this comment

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

This is how asciidoc renders the original format and the two proposed formats:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

formatting fixed

----

*NOTE*
If the specified set of extension ids isn't available in the {prod-short} registry, the workspace starts without the extension.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/id/ID/ throughout.

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed

@benoitf
Copy link
Contributor Author

benoitf commented Apr 7, 2022

who should handle the suggested changes from @rolfedh , @deerskindoll or myself ?


[id="che-theia-plug-ins-YAML"]
== Adding plug-in parameters to `.che/che-theia-plugins.yaml`
You can add extra parameters to a plug-in by modifying the `.che/che-theia-plugins.yaml` file. These modifications include:
Copy link
Contributor

Choose a reason for hiding this comment

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

Bulleted lists don't render as such unless they're preceded by a blank line.

Copy link
Contributor

Choose a reason for hiding this comment

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

formatting fixed

@deerskindoll
Copy link
Contributor

I'll need your help if that's ok with you. I can handle asciidoc fixes but I can't answer the questions regarding devfile v2, air gap mode etc.

. You have the `.che/che-theia-plugins.yaml` file in the root of the GitHub repository.

.Procedure
. Add an `override` section in the `.che/che-theia-plugins.yaml` file under the `id` of the plug-in.
Copy link
Contributor

Choose a reason for hiding this comment

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

add...to

Copy link
Contributor

Choose a reason for hiding this comment

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

wording fixed

. You have the `.che/che-theia-plugins.yaml` file in the root of the GitHub repository.

.Procedure
. Add an `override` section in the `.che/che-theia-plugins.yaml` file under the `id` of the extension.
Copy link
Contributor

@rolfedh rolfedh Apr 7, 2022

Choose a reason for hiding this comment

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

"add...to" here and elsewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

wording fixed

deerskindoll and others added 2 commits April 7, 2022 11:45
Co-authored-by: Rolfe Dlugy-Hegwer <rolfedh@users.noreply.github.com>
Co-authored-by: Rolfe Dlugy-Hegwer <rolfedh@users.noreply.github.com>
[id="visual-studio-code-extensions-in-devfile"]
== Defining Visual Studio Code extension attributes in the devfile

In case it's not possible to add extra files in the GitHub repository, you can define some of the plug-in or extension attributes by inlining them in the devfile. You can use this procedure with both `.vscode/extensions.json` and `.che/che-theia-plugins.yaml` file contents.
Copy link
Contributor

@rolfedh rolfedh Apr 7, 2022

Choose a reason for hiding this comment

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

For minimalism:

  • In case > If ?
  • it's not possible to > you can't ?

Is there a more commonly understood equivalent for "inlining" that preserves the meaning?

Copy link
Contributor

Choose a reason for hiding this comment

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

wording fixed

deerskindoll and others added 2 commits April 7, 2022 11:46
Co-authored-by: Rolfe Dlugy-Hegwer <rolfedh@users.noreply.github.com>
Co-authored-by: Rolfe Dlugy-Hegwer <rolfedh@users.noreply.github.com>
.Procedure
. Add the extension ID to the `.vscode/extensions.json` file. Use a `.` sign to separate the publisher and extension names:
+
*EXAMPLE*
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't find any other instances of *EXAMPLE* in che-docs. Consider adopting the standard code block titles throughout, like this:

.Example <optional descriptive text>

Copy link
Contributor

Choose a reason for hiding this comment

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

formatting fixed

In case it's not possible to add extra files in the GitHub repository, you can define some of the plug-in or extension attributes by inlining them in the devfile. You can use this procedure with both `.vscode/extensions.json` and `.che/che-theia-plugins.yaml` file contents.

=== Inlining `.vscode/extensions.json` file
Use `.vscode/extensions.json` file contents to inline extension attributes in the devfile.
Copy link
Contributor

Choose a reason for hiding this comment

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

Part of the issue with using the adjective, "inline," as a verb is that when you use it next to a noun, as shown here, the reader might experience some semantic noise:
raising the question, "Is a verb missing between "to" and "inline extension attributes?"
Perhaps subsitute it with "add" or "insert?" WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

reworded as "inlining the extension/plug-in attributes"

deerskindoll and others added 2 commits April 7, 2022 13:52
Co-authored-by: Rolfe Dlugy-Hegwer <rolfedh@users.noreply.github.com>
Co-authored-by: Rolfe Dlugy-Hegwer <rolfedh@users.noreply.github.com>
@rolfedh
Copy link
Contributor

rolfedh commented Apr 7, 2022

who should handle the suggested changes from @rolfedh , @deerskindoll or myself ?

My role here is to serve as peer reviewer. I believe @deerskindoll, as the technical writer, should make the updates for language-related issues. I'll at-mention you, @benoitf, for any questions about the software functionality.

deerskindoll and others added 2 commits April 7, 2022 17:22
Copy link
Contributor

@deerskindoll deerskindoll left a comment

Choose a reason for hiding this comment

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

approved

@benoitf
Copy link
Contributor Author

benoitf commented Apr 7, 2022

Merging as all checks are green and it has been approved by Jana and Ilya

@benoitf benoitf merged commit f8447a0 into eclipse-che:master Apr 7, 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