Skip to content

Add che.workspace.plugin_registry_url information to workspace/settings Rest method#11015

Merged
skabashnyuk merged 8 commits intomasterfrom
settingsurl
Sep 4, 2018
Merged

Add che.workspace.plugin_registry_url information to workspace/settings Rest method#11015
skabashnyuk merged 8 commits intomasterfrom
settingsurl

Conversation

@skabashnyuk
Copy link
Copy Markdown
Contributor

@skabashnyuk skabashnyuk commented Aug 31, 2018

What does this PR do?

Add che.workspace.plugin_registry_url information to workspace/settings Rest method
2018-09-03 12 27 30

What issues does this PR fix or reference?

Add public link for che-plugin-registry #10918

Release Notes

Add public link for che-plugin-registry

Docs PR

n/a

@skabashnyuk skabashnyuk requested review from a user and sleshchenko August 31, 2018 13:27
@skabashnyuk skabashnyuk requested a review from l0rd as a code owner August 31, 2018 13:27
@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Aug 31, 2018
@skabashnyuk
Copy link
Copy Markdown
Contributor Author

ci-test

@riuvshin
Copy link
Copy Markdown
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:11015
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@skabashnyuk skabashnyuk changed the title [WIP] Add che.workspace.plugin_registry_url information to workspace/settings Rest method Add che.workspace.plugin_registry_url information to workspace/settings Rest method Sep 3, 2018
@skabashnyuk
Copy link
Copy Markdown
Contributor Author

ci-test

Copy link
Copy Markdown

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

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

LGTM

@garagatyi
Copy link
Copy Markdown

Please, recheck your PR since I merged my PR with walking skeleton and they might conflict/duplicate things.

Copy link
Copy Markdown
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

LGTM

Please take a look my inlined comments

# plugins dependencies to a workspace
che.workspace.plugin_broker.image=eclipse/che-plugin-broker:latest

# Workspace tooling plugins registry endpoint. Should be a valid HTTP URL that includes protocol, port etc.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that includes protocol, port etc. doesn't make property format clear enough. I would be useful to have example value as well.

Joiner.on(",").join(workspaceManager.getSupportedRecipes()),
CHE_WORKSPACE_AUTO_START,
Boolean.toString(cheWorkspaceAutoStart));
if (pluginRegistryUrl == null) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be better to rewrite this method with using Map builder.

    Builder<String, String> settings = ImmutableMap.builder();

    settings.put(Constants.SUPPORTED_RECIPE_TYPES, Joiner.on(",").join(workspaceManager.getSupportedRecipes()));
    settings.put(CHE_WORKSPACE_AUTO_START, Boolean.toString(cheWorkspaceAutoStart));

    if (pluginRegistryUrl != null) {
      settings.put(CHE_WORKSPACE_PLUGIN_REGISTRY_ULR, pluginRegistryUrl);
    }

    return settings.build();

public static final String WS_AGENT_PROCESS_NAME = "CheWsAgent";

public static final String CHE_WORKSPACE_AUTO_START = "che.workspace.auto_start";
public static final String CHE_WORKSPACE_PLUGIN_REGISTRY_ULR =
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add few words to describe in which way this property supposed to be used (exposed by settings method).


# Workspace tooling plugins registry endpoint. Should be a valid HTTP URL that includes protocol, port etc.
# Workspace tooling plugins registry endpoint. Should be a valid HTTP URL.
# Example: http://che-plugin-registry-eclipse-che.192.168.65.2.nip.io/plugins
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

index.json is not in plugins folder, so we should not add it in the URL

@skabashnyuk
Copy link
Copy Markdown
Contributor Author

ci-test

@riuvshin
Copy link
Copy Markdown
Contributor

riuvshin commented Sep 3, 2018

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:11015
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@skabashnyuk
Copy link
Copy Markdown
Contributor Author

@sleshchenko @garagatyi looks like tests are ok. I'm going to merge this pr.

@garagatyi
Copy link
Copy Markdown

👍

@skabashnyuk skabashnyuk merged commit 77a0017 into master Sep 4, 2018
@skabashnyuk skabashnyuk deleted the settingsurl branch September 4, 2018 07:09
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Sep 4, 2018
@benoitf benoitf added this to the 6.11.0 milestone Sep 4, 2018
@garagatyi
Copy link
Copy Markdown

@ashumilova FYI

dmytro-ndp pushed a commit that referenced this pull request Sep 4, 2018
…gs Rest method (#11015)

Add che.workspace.plugin_registry_url information to workspace/settings Rest method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/task Internal things, technical debt, and to-do tasks to be performed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants