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

Deprecate workspaces with Devfile stored as workspace config #13588

Merged
merged 10 commits into from
Jul 3, 2019

Conversation

mkuznyetsov
Copy link
Contributor

Signed-off-by: Mykhailo Kuznietsov mkuznets@redhat.com

What does this PR do?

Deprecate things related to storing workspace converted from devfile as a workspace config:

  • deprecated service method in DevfileService;
  • reworked factory-from-direct-url flow to generate factory with devfile;
  • FactoryDto now has devfile field, and workspace field became non-mandatory;
  • GitHub factories with local referenced components will be automatically resolve it to the referenceContent field of respective components (same as before);
  • small fixes to dashboard to support factory with devfile startup flow;

What issues does this PR fix or reference?

#13423

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>
Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>
…onfig

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>
@Ohrimenko1988
Copy link
Contributor

ci-test

Copy link
Contributor

@metlos metlos left a comment

Choose a reason for hiding this comment

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

LGTM with 1 minor nitpick

// set factory attribute:
let attrs = {factoryId: this.factory.id};

let creationPromise = this.cheAPI.getWorkspace().createWorkspaceFromDevfile(null, devfile, attrs);
Copy link
Contributor

Choose a reason for hiding this comment

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

add the the todo here as well just to be sure?

public FactoryDto withDevfile(DevfileDto devfileDto) {
return factoryDto.withDevfile(devfileDto);
}

@Override
public WorkspaceConfigDto getWorkspace() {
Copy link
Member

Choose a reason for hiding this comment

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

Please add @nullable and describe when it may be such. The same for workspace config field

@sleshchenko
Copy link
Member

As far as I know, factory used to generate a name if workspace with such name already exist, I tried to test your PR and saw this error
Screenshot_20190621_140051

Please double check if it is not a regression. I mean against accepting a factory with workspace config but against the deprecated way of accepting factory with devfile via DevfileService.

@sleshchenko
Copy link
Member

Have you seen that the following message is shown on a first stage of accepting a factory from Devfile?
Screenshot_20190621_141034
I think it's not expected behavior, please consider fixing it or create a separate issue.

@sleshchenko
Copy link
Member

sleshchenko commented Jun 21, 2019

One more issue I faced during manual testing: my repository had an incorrect devfile (apiVersion was 0.0.1 instead of 1.0.0) and I saw an exception from Che Server only on a phase of workspace starting, the factory was successfully accepted and in the result I had a created incorrect workspace that I was not able to start.
Screenshot_20190621_141311
Screenshot_20190621_141326
Please check factory accepting when Devfile is invalid, devfile should be validated on a phase of a factory accepting but not after workspace creating.
P.S. You can use the following Devfile for testing https://github.com/raw/sleshchenko/spring-petclinic/master/devfile.yaml

@che-bot
Copy link
Contributor

che-bot commented Jun 21, 2019

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

@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/task Internal things, technical debt, and to-do tasks to be performed. labels Jun 27, 2019
Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>
@mkuznyetsov
Copy link
Contributor Author

now behaviour of accepting direct URL factories will now go as follows:

  • if repository contains Devfile -> create workspace from this Devfile
  • else, if it contains factory.json -> create factory with this factory.json
  • else, factory will be created with base generated devfile, instead of workspace-config based workspace

also @l0rd and others, there would be slight regression, if I would attempt to use workspace from devfiles by default, for factories without devfile.yaml - there wouldn't be support for "keepDir" param for specifying subdirectory of a repository of a cloned project (devfile projects don't support "keepDir", while

Since it's a feature only used by Che 6 factory, I'd suggest fixing "keepDir" in a separate issue (which would require changes to model, schemas, etc. and probably for Theia as well, which is something that @sunix should know about )

…ctory.json

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>
@skabashnyuk
Copy link
Contributor

@mkuznyetsov yes. please create issues for keepDir support

@Ohrimenko1988
Copy link
Contributor

ci-test

@che-bot
Copy link
Contributor

che-bot commented Jul 2, 2019

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

…l or factory.json

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>
@mkuznyetsov
Copy link
Contributor Author

ci-test

@che-bot
Copy link
Contributor

che-bot commented Jul 2, 2019

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

…ile.yaml or factory.json

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>
@mkuznyetsov
Copy link
Contributor Author

ci-test

@che-bot
Copy link
Contributor

che-bot commented Jul 2, 2019

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

@mkuznyetsov
Copy link
Contributor Author

ci-test

@che-bot
Copy link
Contributor

che-bot commented Jul 3, 2019

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

@SkorikSergey
Copy link
Contributor

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

@mkuznyetsov mkuznyetsov merged commit 8d3cc80 into master Jul 3, 2019
@mkuznyetsov mkuznyetsov deleted the che-13423 branch July 26, 2019 13:31
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. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants