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

This PR adds initial improvements that allow developer easier to start #3

Merged
merged 9 commits into from
Jan 11, 2020
Merged

This PR adds initial improvements that allow developer easier to start #3

merged 9 commits into from
Jan 11, 2020

Conversation

sleshchenko
Copy link
Member

@sleshchenko sleshchenko commented Dec 26, 2019 •
edited
Loading

What does this PR do?

This PR adds initial improvements that allow developer easier to start:

  1. There is a bug in fsnotify transitive dependency that breaks Go Dep because of the empty versions list. As a workaround, direct dependency on the source archive should be added.
  2. Latest plugin broker has additional methods that were not implemented in workspace CRD before, so it's fixed.
  3. WorkspaceCRD controller requires some scripts that operator sdk generate during initializing, now they are committed.
  4. For some reason, dockerimage build fails on Ubuntu when bazel is installer with yum, so, alternative implementation way is used - dnf works on Ubuntu and Fedora as well.
  5. It was quite difficult to understand where I need to take a look to understand overall controller architecture, I believe it should be easier to navigate and learn overall architecture with separate packages.
  6. Added initial instructions on how to run and test Workspace CRD controller. It also includes fixes for default values.
  7. Format the code with gofmt -w ./..

What issues does this PR fix or reference?

It was done during eclipse-che/che#15522

@sleshchenko
Fix fsnotify transitive dependency
6dce460 
Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
@sleshchenko
Make code compatible with the latest plugin broker
9f46f9b 
Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
@sleshchenko sleshchenko self-assigned this Dec 26, 2019
@sleshchenko sleshchenko force-pushed the initialImprovements branch 2 times, most recently from 1d00c24 to 2353e93 Compare December 28, 2019 14:15
amisevsk
amisevsk reviewed Jan 6, 2020
View reviewed changes
Copy link
Collaborator

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

A few comments from a relatively quick read, I'll take another look later.

build/bin/user_setup Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
deploy/controller_config.yaml Show resolved Hide resolved
pkg/controller/workspace/config/config.go Outdated Show resolved Hide resolved
pkg/controller/workspace/log/log.go Outdated Show resolved Hide resolved
pkg/controller/workspace/k8s/utilities.go Outdated Show resolved Hide resolved
pkg/controller/workspace/status.go Outdated Show resolved Hide resolved
pkg/controller/workspace/utils/utilities.go Outdated Show resolved Hide resolved
davidfestal
davidfestal reviewed Jan 7, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
build/Dockerfile Show resolved Hide resolved
build/bin/entrypoint Outdated Show resolved Hide resolved
build/bin/user_setup Show resolved Hide resolved
build/bin/user_setup Show resolved Hide resolved
deploy/registry/local/ingress.yaml Outdated Show resolved Hide resolved
deploy/registry/local/service.yaml Outdated Show resolved Hide resolved
pkg/controller/workspace/config/config.go Outdated Show resolved Hide resolved
pkg/controller/workspace/k8s/utilities.go Outdated Show resolved Hide resolved
pkg/controller/workspace/k8s/convertToLowLevelObjects.go Outdated Show resolved Hide resolved
@sleshchenko
Add missing scripts
08477cf 
Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
@sleshchenko
Fix dockerimage build on ubuntu
74929de 
Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
@sleshchenko
Restructure packages
85d2da8 
Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
l0rd
l0rd reviewed Jan 9, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
l0rd
l0rd reviewed Jan 9, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
l0rd
l0rd reviewed Jan 9, 2020
View reviewed changes
README.md Outdated
### Run controller within K8s/OS cluster
1. `kubectl apply -f ./deploy/crds`
2. `kubectl create namespace devworkspace-controller`
3. For K8s cluster you must to make sure that right domain is set in `./deploy/controller_config.yaml` and `./deploy/registry/local/ingress.yaml`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you specifying "For K8s cluster" here? This section is about K8s/OS, if this step is for K8s only (not OS) that's ok to be explicit but I think that's for both so I would only mention "Make sure that the right domain is set in....".

Copy link
Collaborator

@davidfestal davidfestal Jan 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For the OpenShift case we don't need to provide this information. It is retrieved automatically at start of the controller, at least in the controller config map.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Actually, I did not try to run it on OpenShift cluster, maybe Route instead of Ingress is needed. For time being I've renamed section to Run controller within K8s cluster and then it's needed to test and fill in instructions for OpenShift Cluster

l0rd
l0rd reviewed Jan 9, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
amisevsk
amisevsk reviewed Jan 9, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
amisevsk
amisevsk reviewed Jan 9, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
@sleshchenko
Add initial intructions on how to run Workspace CRD Controller
1ce344f 
Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
@sleshchenko
Do not install bazel
3496668 
Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
@sleshchenko
Add Eclipse Public License - v2.0 License
a8c4e0c 
Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
@sleshchenko
Format the code
12bc188 
Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
amisevsk
amisevsk approved these changes Jan 10, 2020
View reviewed changes
Copy link
Collaborator

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

I went through the provided readme and was able to get a workspace up and running -- LGTM.

@sleshchenko sleshchenko merged commit 544ea89 into devfile:master Jan 11, 2020
@sleshchenko sleshchenko changed the title This PR adds initial improvements that allows developer easier to start This PR adds initial improvements that allow developer easier to start Jan 11, 2020
@sleshchenko sleshchenko deleted the initialImprovements branch January 11, 2020 08:35
@sleshchenko sleshchenko mentioned this pull request Jan 23, 2020
Closed
JPinkney pushed a commit to JPinkney/che-workspace-operator that referenced this pull request May 13, 2021
@metlos
Merge pull request devfile#3 from metlos/ci
85becba 
Make CI pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@l0rd l0rd l0rd left review comments

@davidfestal davidfestal davidfestal approved these changes

@amisevsk amisevsk amisevsk approved these changes

Assignees

@sleshchenko sleshchenko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sleshchenko @l0rd @davidfestal @amisevsk