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.

Sign up for GitHub

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

Jump to bottom

Implement mountSources attribute for plugin containers #12527

Merged
merged 1 commit into from
Feb 1, 2019
Merged

Implement mountSources attribute for plugin containers #12527

merged 1 commit into from
Feb 1, 2019

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Jan 28, 2019 •
edited
Loading

What does this PR do?

Implement automatic mounting of project sources volume into /projects
by specifying the mountSources attribute in che-plugin.yaml.

What issues does this PR fix or reference?

#12510

Release Notes

Docs PR

N/A

@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Jan 28, 2019
@metlos
Copy link
Contributor Author

metlos commented Jan 28, 2019

ci-test

@metlos
Copy link
Contributor Author

metlos commented Jan 28, 2019

Note that eclipse-che/che-plugin-broker#27 is associated with this PR.

sleshchenko
sleshchenko reviewed Jan 28, 2019
View reviewed changes
Copy link
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.

Note that eclipse-che/che-plugin-broker#27 is associated with this PR.

then please make sure that will be merged, released and brokers images tags will be updated in this PR

@che-bot
Copy link
Contributor

che-bot commented Jan 28, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12527
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

garagatyi
garagatyi approved these changes Jan 29, 2019
View reviewed changes
Copy link

@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.

Looks good

garagatyi
garagatyi reviewed Jan 29, 2019
View reviewed changes
...main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/MachineResolver.java Outdated
}

for (Volume volume : container.getVolumes()) {
if (volume.getName().equals(PROJECTS_VOLUME_NAME)) {
Copy link

Choose a reason for hiding this comment

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

According to this current plugins would not allow starting a workspace. Can you elaborate on how are you going to handle that?

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 thought I would not handle that because since Che7 is not released, we don't give any guarantees on the "API stability".

Copy link

Choose a reason for hiding this comment

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

We have Che 7 running on OSIO. So...
And breaking it even temporarily for users that use different versions of Che is also something we might want to avoid

Copy link

Choose a reason for hiding this comment

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

@skabashnyuk @ibuziuk we should address that not to break prod

Copy link

Choose a reason for hiding this comment

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

This PR, in particular, would not break Che on OSIO. But it would break Che 7 workspaces on nightly Che as far as I understand. And Selenium tests show that Che 7 test doesn't pass.
If we do a change in Theia and rollout that on OSIO without rolling out upstream version with this PR we would break Che on OSIO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code now allows to mount project to /projects manually, but will fail if the mount point is different. I.e. it actively prevents a plugin from mounting sources anywhere else but /projects or rather wherever the che.workspace.projects.storage configuration property points.

garagatyi
garagatyi suggested changes Jan 29, 2019
View reviewed changes
Copy link

@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.

Please ensure that Che 7 workspace start is not broken by this PR

garagatyi
garagatyi approved these changes Jan 30, 2019
View reviewed changes
Copy link

@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.

Looks good

@garagatyi
Copy link

Please, do not merge without passing functional tests

@metlos
Copy link
Contributor Author

metlos commented Jan 30, 2019

ci-build

@garagatyi
Copy link

@metlos I doubt ci-build will pass. You have a merge conflict.
FYI functional test can be run with ci-test

@metlos
Copy link
Contributor Author

metlos commented Jan 30, 2019

ci-test

@che-bot
Copy link
Contributor

che-bot commented Jan 30, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12527
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@garagatyi
Copy link

@metlos @sleshchenko @skabashnyuk Can we proceed with this PR review/tests? My PR #12560 depends on this one because of new mountSources attribute sent from brokers to WS master

sleshchenko
sleshchenko approved these changes Jan 31, 2019
View reviewed changes
Copy link
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

skabashnyuk
skabashnyuk approved these changes Jan 31, 2019
View reviewed changes
Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

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

Can you specify some examples in the PR description?

@garagatyi
Copy link

ci-test

@garagatyi
Copy link

@artaleks9 Please, take a look

@garagatyi garagatyi mentioned this pull request Jan 31, 2019
Merged
@artaleks9
Copy link
Contributor

So, frankly speaking, the report is quite good.
Some tests are fail by same reason on the master, some, I guess, via infrastructure.
Let's wait the next report to be sure.

@che-bot
Copy link
Contributor

che-bot commented Jan 31, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12527
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@artaleks9
Copy link
Contributor

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/1495/) doesn't show any regression against this Pull Request.

@metlos
Implement automatic mounting of project sources volume into /projects
92c2850 
by specifying the mountSources attribute in che-plugin.yaml.

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@metlos
Copy link
Contributor Author

metlos commented Jan 31, 2019

I squashed the commits into one, because there was some going back-and-forth before we settled on the final shape of things.

@metlos
Copy link
Contributor Author

metlos commented Jan 31, 2019

ci-test

@garagatyi
Copy link

@metlos I think it is OK to merge PR without the extra run of Selenium tests

@che-bot
Copy link
Contributor

che-bot commented Jan 31, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12527
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@metlos metlos merged commit 6e8f537 into eclipse-che:master Feb 1, 2019
@che-bot che-bot added this to the 6.18.0 milestone Feb 1, 2019
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 1, 2019
@metlos metlos mentioned this pull request Feb 5, 2019
Closed
@che-bot
Copy link
Contributor

che-bot commented Feb 5, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12527
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skabashnyuk skabashnyuk skabashnyuk approved these changes

@garagatyi garagatyi garagatyi approved these changes

@sleshchenko sleshchenko sleshchenko approved these changes

@riuvshin riuvshin Awaiting requested review from riuvshin

Assignees
No one assigned
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Milestone
6.18.0
Development

Successfully merging this pull request may close these issues.
6 participants
@metlos @che-bot @garagatyi @artaleks9 @skabashnyuk @sleshchenko