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

feat and bugfixes: update kb and add edit command to support multigroups #4156

Merged

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Nov 1, 2020

Description of the change:

Motivation for the change:

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@@ -17,7 +17,6 @@ limitations under the License.
package v1alpha1

import ("errors"
"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change in the scaffold is because of the bugfix create webhook subcommand for the defaulting webhooks by removing import that is not used. More info: kubernetes-sigs/kubebuilder#1718

hack/check-links.sh Outdated Show resolved Hide resolved
@joelanford
Copy link
Member

Just double checking: Is this edit plugin/CLI project type aware? Just want to make sure that operator-sdk edit is only added to the CLI when the project type is Go.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2020
@camilamacedo86

This comment has been minimized.

Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

I found 2 nits, but those can be fixed in a follow on PR if need be. Overall I looks fine to me.

version: 2
...
```
Before we start to create the APIs, check if your project has more than one group such as : `foo.example.com/v1` and `crew.example.com/v1`. If you intend to still working on with multiple groups in your project, then to change the layout of your project to support Multi-Group run the command `operator-sdk edit --multigroup=true`
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I know it was already like this but I think it should read:
"If you intend to work with multiple groups in your project, then to change the project's layout to support Multi-Group, run the command operator-sdk edit --multigroup=true.


```YAML
domain: example.com
layout: go.kubebuilder.io/v2
multigroup: true
...
```
For multi-group projects, the API Go type files are created under `apis/<group>/<version>/` and the controllers under `controllers/<group>/`.
For multi-group projects, the API Go type files are created under `apis/<group>/<version>/` and the controllers under `controllers/<group>/` and then, the Dockerfile will be also updated accordingly. For further information see the [Single Group to Multi-Group][multigroup-kubebuilder-doc]
Copy link
Member

Choose a reason for hiding this comment

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

nit: the Dockerfile will be updated accordingly.

@jmrodri
Copy link
Member

jmrodri commented Nov 11, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2020
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Some nits

Comment on lines 5 to 6
(go/v2) Added the command `òperator-sdk edit` which allows users edit the project layout to support Multi-Group.
More info: https://book.kubebuilder.io/migration/multi-group.html
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(go/v2) Added the command `òperator-sdk edit` which allows users edit the project layout to support Multi-Group.
More info: https://book.kubebuilder.io/migration/multi-group.html
(go/v2) Added the command `òperator-sdk edit` which allows users edit the project layout to support [multi-group](https://book.kubebuilder.io/migration/multi-group.html).

Copy link
Member

Choose a reason for hiding this comment

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

Also Mulit-Group really should be lower-cased, as it is not a proper noun in this context.

Comment on lines 10 to 11
(go/v2) Fix `create webhook` subcommand for the defaulting webhooks by removing import that is not used.
More info: https://github.com/kubernetes-sigs/kubebuilder/pull/1718
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(go/v2) Fix `create webhook` subcommand for the defaulting webhooks by removing import that is not used.
More info: https://github.com/kubernetes-sigs/kubebuilder/pull/1718
(go/v2) Removed unused import for defaulting webhooks scaffolded by `create webhook` ([kubebuilder#1718](https://github.com/kubernetes-sigs/kubebuilder/pull/1718)).

Comment on lines 15 to 16
(go/v2) Fix `copyright` info on the license of the generated files when the owner is not informed.
More info: https://github.com/kubernetes-sigs/kubebuilder/pull/1749
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(go/v2) Fix `copyright` info on the license of the generated files when the owner is not informed.
More info: https://github.com/kubernetes-sigs/kubebuilder/pull/1749
(go/v2) Allow owner to not be specified in generated licenses ([kubebuilder#1749](https://github.com/kubernetes-sigs/kubebuilder/pull/1749)).

@@ -61,16 +61,15 @@ Read the [operator scope][operator_scope] documentation on how to run your opera

### Multi-Group APIs

Before creating an API and controller, consider if your operator's API requires multiple [groups][API-groups].
If yes then add the line `multigroup: true` in the `PROJECT` file which should look like the following:
Before creating an API and controller, consider if your operator's API requires multiple [groups][API-groups]. Then to change the layout of your project to support Multi-Group run the command `operator-sdk edit --multigroup=true`. It will update the `PROJECT` file which should look like the following:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Before creating an API and controller, consider if your operator's API requires multiple [groups][API-groups]. Then to change the layout of your project to support Multi-Group run the command `operator-sdk edit --multigroup=true`. It will update the `PROJECT` file which should look like the following:
Before creating an API and controller, consider if your operator requires multiple API [groups][api-groups]. Then to change the layout of your project to support multi-group run the command `operator-sdk edit --multigroup=true`. It will update the `PROJECT` file which should look like the following:


```YAML
domain: example.com
layout: go.kubebuilder.io/v2
multigroup: true
...
```
For multi-group projects, the API Go type files are created under `apis/<group>/<version>/` and the controllers under `controllers/<group>/`.
For multi-group projects, the API Go type files are created under `apis/<group>/<version>/` and the controllers under `controllers/<group>/` and then, the Dockerfile will be also updated accordingly. For further information see the [Single Group to Multi-Group][multigroup-kubebuilder-doc]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For multi-group projects, the API Go type files are created under `apis/<group>/<version>/` and the controllers under `controllers/<group>/` and then, the Dockerfile will be also updated accordingly. For further information see the [Single Group to Multi-Group][multigroup-kubebuilder-doc]
For multi-group projects, the API Go type files are created under `apis/<group>/<version>/` and the controllers under `controllers/<group>/` and then, the Dockerfile will be also updated accordingly. For further information see the [multi-group migration doc][multigroup-kubebuilder-doc]

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2020
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@camilamacedo86 camilamacedo86 merged commit 0859bd2 into operator-framework:master Nov 11, 2020
@camilamacedo86 camilamacedo86 deleted the add-edit-plugin branch November 11, 2020 23:01
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
…ups (operator-framework#4156)

**Description of the change:**
- Update kubebuilder dependency (See: kubernetes-sigs/kubebuilder@36124ae...9c02d55)
- Add edit command which allows multigroup support
- Align SDK with Kubebuilder and address bug fixes

**Motivation for the change:**
- Solve tech-debt and keep both projects aligned
- Close the issue: operator-framework#3678 and provide the same facility that has in upstream to change the layout to support multigroup  
- Close the issue: operator-framework#4157

Signed-off-by: reinvantveer <rein.van.t.veer@geodan.nl>
rearl-scwx pushed a commit to rearl-scwx/operator-sdk that referenced this pull request Feb 5, 2021
…ups (operator-framework#4156)

**Description of the change:**
- Update kubebuilder dependency (See: kubernetes-sigs/kubebuilder@36124ae...9c02d55)
- Add edit command which allows multigroup support
- Align SDK with Kubebuilder and address bug fixes

**Motivation for the change:**
- Solve tech-debt and keep both projects aligned
- Close the issue: operator-framework#3678 and provide the same facility that has in upstream to change the layout to support multigroup  
- Close the issue: operator-framework#4157

Signed-off-by: rearl <rearl@secureworks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants