Skip to content
This repository has been archived by the owner on Oct 9, 2024. It is now read-only.

feat: add execution environment auto completion #42

Merged
merged 13 commits into from
Oct 5, 2021

Conversation

ganeshrn
Copy link
Member

  • Add support for execution environment (EE) configuration
    options received from client.
    Example client settings.json for EE options (default)

    {
     "ansible.executionEnvironment.enabled": false,
     "ansible.executionEnvironment.image": "quay.io/ansible/ansible-navigator-demo-ee:0.6.0",
     "ansible.executionEnvironment.containerEngine": "auto",
     "ansible.executionEnvironment.pullPolicy: "missing"
    }
    
  • Add executionEnvironment service to handle initilization of EE
    and pulling plugins from within EE into cache path on local system

  • In case EE is enabled update docsLibrary service to read plugin
    docs from local cache path after executionEnvironment service
    is initialized

  • Add commandRunner utility to run command on local host or within
    EE based on settings passed from client

  • Update ansibleConfig service to use commandRunner utility
    to run ansible-config and related commands

@ganeshrn ganeshrn force-pushed the executionEnvironment branch 5 times, most recently from c5bb029 to 39281b7 Compare September 27, 2021 05:16
ganeshrn added a commit to ganeshrn/vscode-ansible that referenced this pull request Sep 27, 2021
*  Add setting in package.json file to configure execution
   environment related setting.
   Example client `settings.json` for EE options (default)
   ```
   {
    "ansible.executionEnvironment.enabled": false,
    "ansible.executionEnvironment.image": "quay.io/ansible/ansible-navigator-demo-ee:0.6.0",
    "ansible.executionEnvironment.containerEngine": "auto",
    "ansible.executionEnvironment.pullPolicy: "missing"
   }
   ```

   Related to ansible-langauge-server PR ansible/ansible-language-server#42
@ganeshrn ganeshrn requested review from a team and tomaciazek September 27, 2021 05:31
@webknjaz

This comment has been minimized.

@ganeshrn ganeshrn requested a review from a team September 27, 2021 08:18
@ssbarnea ssbarnea added the enhancement New feature or request label Sep 27, 2021
Copy link
Contributor

@tomaciazek tomaciazek left a comment

Choose a reason for hiding this comment

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

Nice work abstracting away execution environment and brilliant caching mechanism!

I noticed that you're missing semicolons in many of the newly added lines. I'm not sure whether it was enforced by eslint, but Prettier was definitely configured to fill them in, it should be enough to install the Prettier extension and re-save the files.

Also, some inline and overview docstrings would be nice.

src/services/ansibleConfig.ts Outdated Show resolved Hide resolved
src/utils/commandRunner.ts Outdated Show resolved Hide resolved
src/services/executionEnvironment.ts Outdated Show resolved Hide resolved
src/services/executionEnvironment.ts Outdated Show resolved Hide resolved
src/services/executionEnvironment.ts Outdated Show resolved Hide resolved
src/services/executionEnvironment.ts Outdated Show resolved Hide resolved
src/services/executionEnvironment.ts Outdated Show resolved Hide resolved
src/interfaces/extensionSettings.ts Outdated Show resolved Hide resolved
src/services/executionEnvironment.ts Outdated Show resolved Hide resolved
src/services/docsLibrary.ts Outdated Show resolved Hide resolved
@ganeshrn
Copy link
Member Author

/cc @yaegassy For review from coc.nvim extension perspective.

@priyamsahoo
Copy link
Contributor

@ganeshrn, I don't think it's a great idea to use this.connection.window.showInformationMessage() or this.connection.window.showErrorMessage() in the language-server code base. I believe these are vscode APIs and we don't want to use them in the server code since server is meant to be used by other text editors and IDEs as well.

A similar issue is raised here: #15

*  Add support for execution environment (EE) configuration
   options received from client.
   Example client `settings.json` for EE options (default)
   ```
   {
    "ansible.executionEnvironment.enabled": false,
    "ansible.executionEnvironment.image": "quay.io/ansible/ansible-navigator-demo-ee:0.6.0",
    "ansible.executionEnvironment.containerEngine": "auto",
    "ansible.executionEnvironment.pullPolicy: "missing"
   }
   ```

*  Add executionEnvironment service to handle initilization of EE
   and pulling plugins from within EE into cache path on local system

*  In case EE is enabled update docsLibrary service to read plugin
   docs from local cache path after executionEnvironment service
   is initialized

*  Add commandRunner utility to run command on local host or within
   EE based on settings passed from client

*  Update ansibleConfig service to use commandRunner utility
   to run ansible-config and related commands
*  Add semicolon and fix indetation using Prettier extesnion
*  Use vscode-uri package to get document path in commandRunner utility
*  Add progess status for downloaing EE
*  Change user message to log messeage at all applicable places
*  Add comments on code for better readability
*  Simplify logic to add `plugins` folder in `site-packages` path
*  Fix type on executionEnvironment service
*  Remove `--interactive` flag from container run command
@ganeshrn
Copy link
Member Author

@tomaciazek Thanks for your detailed review. I have fixed most of them in my recent commit. Please re-review it.

I noticed that you're missing semicolons in many of the newly added lines. I'm not sure whether it was enforced by eslint, but Prettier was definitely configured to fill them in, it should be enough to install the Prettier extension and re-save the files.

Since I come from python world I tend to inherently ignore semicolons :-)
On a serious note your suggestion of using prettier extension worked like a charm.

@ganeshrn
Copy link
Member Author

@priyamsahoo Based on the API documentation for the method it doesn't seem like it is specific to vscode.

    showErrorMessage(message: string): void;
    showErrorMessage<T extends MessageActionItem>(message: string, ...actions: T[]): Promise<T | undefined>;
    /**
     * Shows a warning message in the client's user interface. Depending on the client this might
     * be a modal dialog with a confirmation button or a notification in a notification center
     *
     * @param message The message to show.
     * @param actions Possible additional actions presented in the user interface. The selected action
     *  will be the value of the resolved promise
     */
    showWarningMessage(message: string): void;
    showWarningMessage<T extends MessageActionItem>(message: string, ...actions: T[]): Promise<T | undefined>;
    /**
     * Shows an information message in the client's user interface. Depending on the client this might
     * be a modal dialog with a confirmation button or a notification in a notification center
     *
     * @param message The message to show.
     * @param actions Possible additional actions presented in the user interface. The selected action
     *  will be the value of the resolved promise
     */
    showInformationMessage(message: string): void;
    showInformationMessage<T extends MessageActionItem>(message: string, ...actions: T[]): Promise<T | undefined>;

In any case the current test scope if for vscode extension. If in future we figure out it doesn't work with other clients we can fix it in appropriate way as part of Issue #15 for all the usages in the project including the messages added as part of this PR. IMO your comment is out of scope for this particular PR.

src/utils/imagePuller.ts Outdated Show resolved Hide resolved
src/services/executionEnvironment.ts Outdated Show resolved Hide resolved
src/services/executionEnvironment.ts Outdated Show resolved Hide resolved
src/services/executionEnvironment.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@tomaciazek tomaciazek left a comment

Choose a reason for hiding this comment

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

We should avoid force-pushing once the branch is in PR review stage. For instance, I can't browse changes since my last review. Merge-commits are perfectly fine within branches - they are going to be squashed on merge to main anyway.

@tomaciazek
Copy link
Contributor

Sorry, GitHub pulled a trick on me again and some replies to earlier comments ended up duplicated as standalone comments.

@ganeshrn
Copy link
Member Author

ganeshrn commented Sep 29, 2021

@tomaciazek Fixed your another set of review comments. Some of them are still marked unresolved and I have provided my feedback on them. Please re-review the PR.

@ganeshrn
Copy link
Member Author

We should avoid force-pushing once the branch is in PR review stage. For instance, I can't browse changes since my last review. Merge-commits are perfectly fine within branches - they are going to be squashed on merge to main anyway.

My bad. Something was not right in my local branch. Have fixed it now.

ganeshrn added a commit to ganeshrn/vscode-ansible that referenced this pull request Sep 30, 2021
*  Add setting in package.json file to configure execution
   environment related setting.
   Example client `settings.json` for EE options (default)
   ```
   {
    "ansible.executionEnvironment.enabled": false,
    "ansible.executionEnvironment.image": "quay.io/ansible/ansible-navigator-demo-ee:0.6.0",
    "ansible.executionEnvironment.containerEngine": "auto",
    "ansible.executionEnvironment.pullPolicy: "missing"
   }
   ```

   Related to ansible-langauge-server PR ansible/ansible-language-server#42
ganeshrn added a commit to ansible/vscode-ansible that referenced this pull request Sep 30, 2021
* feat(ee): this is a backup commit

*  Add support for execution environment

* feat: add setting options for execution environment

*  Add setting in package.json file to configure execution
   environment related setting.
   Example client `settings.json` for EE options (default)
   ```
   {
    "ansible.executionEnvironment.enabled": false,
    "ansible.executionEnvironment.image": "quay.io/ansible/ansible-navigator-demo-ee:0.6.0",
    "ansible.executionEnvironment.containerEngine": "auto",
    "ansible.executionEnvironment.pullPolicy: "missing"
   }
   ```

   Related to ansible-langauge-server PR ansible/ansible-language-server#42

* chore: remove stale edits from package.json file

* chore: fix review comment

* chore: fix review comment and order setting correctly

* chore: order setting alphabetically
@priyamsahoo
Copy link
Contributor

priyamsahoo commented Sep 30, 2021

@priyamsahoo Based on the API documentation for the method it doesn't seem like it is specific to vscode.

    showErrorMessage(message: string): void;
    showErrorMessage<T extends MessageActionItem>(message: string, ...actions: T[]): Promise<T | undefined>;
    /**
     * Shows a warning message in the client's user interface. Depending on the client this might
     * be a modal dialog with a confirmation button or a notification in a notification center
     *
     * @param message The message to show.
     * @param actions Possible additional actions presented in the user interface. The selected action
     *  will be the value of the resolved promise
     */
    showWarningMessage(message: string): void;
    showWarningMessage<T extends MessageActionItem>(message: string, ...actions: T[]): Promise<T | undefined>;
    /**
     * Shows an information message in the client's user interface. Depending on the client this might
     * be a modal dialog with a confirmation button or a notification in a notification center
     *
     * @param message The message to show.
     * @param actions Possible additional actions presented in the user interface. The selected action
     *  will be the value of the resolved promise
     */
    showInformationMessage(message: string): void;
    showInformationMessage<T extends MessageActionItem>(message: string, ...actions: T[]): Promise<T | undefined>;

In any case the current test scope if for vscode extension. If in future we figure out it doesn't work with other clients we can fix it in appropriate way as part of Issue #15 for all the usages in the project including the messages added as part of this PR. IMO your comment is out of scope for this particular PR.

@ganeshrn, I think the API implementation belongs to https://github.com/microsoft/vscode/blob/9a21b53/src/vs/vscode.d.ts#L8848, which is specefic to vscode (I may be wrong).

But sure, if we see trouble executing this statement in other IDEs, we can have fixes for this in separate PR.

Copy link
Contributor

@priyamsahoo priyamsahoo left a comment

Choose a reason for hiding this comment

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

LGTM !

@cidrblock cidrblock self-requested a review September 30, 2021 14:19
Copy link
Contributor

@tomaciazek tomaciazek left a comment

Choose a reason for hiding this comment

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

Good job!

@ganeshrn ganeshrn enabled auto-merge (squash) October 5, 2021 02:01
@ganeshrn ganeshrn merged commit cefd599 into ansible:main Oct 5, 2021
@ganeshrn
Copy link
Member Author

ganeshrn commented Oct 5, 2021

Good job!

Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants