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

✨ store all config info (go/v3-alpha) #1696

Closed

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Sep 25, 2020

Description

Store in the PROJECT file:

  • webhooks data
  • if has or not an api scaffolded
  • if has or not a controller scaffolded
  • API domain and endpoint when it is not the default form
  • Plural form when it is not the default form

Refractories/Cleanups suggested:

  • Rename Domain in the internal code for QualifiedGroup
  • Rename Package for Endpoint
  • Rename GVK to ResourceData since it does not return only the GKV
  • Replace doResource with doAPI only for v3 internal code.

PS.: Note the plugin v2 internal code was changed/updated where only it would be mandatory in order to move forward with the changes. The idea is we ensure and not risk introduces breaking changes or bugs into it since we are closer to deprecated it.

Regards the tests:

  • Add new tests to cover the config
  • Add new api scaffolded in the testdata for multigroup projects to cover better the k8s core types option.

Example

Following an example with the edge cases and that cover nearly all scenarios.

domain: testproject.org
layout: go.kubebuilder.io/v3-alpha
multigroup: true
projectName: project-v3-multigroup
repo: sigs.k8s.io/kubebuilder/testdata/project-v3-multigroup
resources:
- controller: true
  domain: k8s.io
  endpoint: k8s.io/api/authentication/v1
  group: authentication
  kind: TokenReview
  version: v1
- api:
    crdVersion: v1
    namespaced: true
  controller: true
  domain: testproject.org
  kind: Lakers
  version: v1
  webhooks:
    defaulting: true
    validation: true
    webhookVersion: v1
version: 3-alpha

Motivation

  • Allow we have all project config into the config which can, for example, grant we re-create the project based on it.
  • keep the consistency across the project
  • Allow we easily manage diff versions of apiVersions for webhooks and CRDs. See that the apiVersion fits very well as an attribute of its models. So, we could store the apiVersion which would allow how the tool should dealing with each case scenario. See: ⚠️ (go/v3-alpha) default to v1 CRDs and webhooks #1644 (comment)

Close: #1826

/hold

If we do not able o ve forward with #1869 instead of this one. Then, here would be required to do a few changes :

  • Use the nomenclature Path instead of endpoint
  • Do not store the domain when it is a core value.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 25, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 25, 2020
@camilamacedo86 camilamacedo86 changed the title ✨ store webhooks on Project file and ensure that the resources behaviours are applied to them as well (only v3+) wIP ✨ store webhooks on Project file and ensure that the resources behaviours are applied to them as well (only v3+) Sep 25, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 25, 2020
@camilamacedo86 camilamacedo86 changed the title wIP ✨ store webhooks on Project file and ensure that the resources behaviours are applied to them as well (only v3+) ✨ store webhooks on Project file and ensure that the resources behaviours are applied to them as well (only v3+) Sep 25, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2020
@camilamacedo86 camilamacedo86 changed the title ✨ store webhooks on Project file and ensure that the resources behaviours are applied to them as well (only v3+) ✨ store webhooks config info and ensure for them the same resources behaviours (only v3+) Sep 25, 2020
@camilamacedo86 camilamacedo86 force-pushed the webhooks-store branch 4 times, most recently from ca5dbeb to 14e00cc Compare September 25, 2020 19:18
Copy link
Contributor

@gabbifish gabbifish left a comment

Choose a reason for hiding this comment

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

I have some minor nits and a suggestion for an extra validity check, otherwise this looks great!! Your tests are so comprehensive 😍

pkg/model/config/config.go Outdated Show resolved Hide resolved
pkg/model/config/config_test.go Outdated Show resolved Hide resolved
pkg/model/config/config_test.go Outdated Show resolved Hide resolved
pkg/model/config/config_test.go Outdated Show resolved Hide resolved
pkg/plugin/v3/webhook.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2020
@camilamacedo86 camilamacedo86 changed the title ✨ store webhooks config info and ensure for them the same resources behaviours (only v3+) ✨ store webhooks config info and add make run option (only v3+) Oct 1, 2020
@camilamacedo86 camilamacedo86 changed the title ✨ store webhooks config info and add make run option (only v3+) ✨ store webhooks config info and make options (only v3+) Oct 1, 2020
@camilamacedo86 camilamacedo86 changed the title ✨ store webhooks config info and make options (only v3+) ✨ store webhooks config info and add make run and force options (only v3+) Oct 1, 2020
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Oct 1, 2020

/hold cancel

@gabbifish fell free to re-check

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2020
Copy link
Contributor

@gabbifish gabbifish left a comment

Choose a reason for hiding this comment

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

Some tiny nits for gkv->gvk--once those are done, I'll approve! :D

pkg/model/config/config.go Outdated Show resolved Hide resolved
pkg/model/config/config.go Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 changed the title ✨ store webhooks config info (go/v3-alpha) ✨ store all config info (go/v3-alpha) Dec 2, 2020
@camilamacedo86
Copy link
Member Author

/hold

we are trying to reach a consensus to move forward here.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2020
@camilamacedo86 camilamacedo86 force-pushed the webhooks-store branch 2 times, most recently from 3390cfe to a6bbd90 Compare December 2, 2020 17:15
@camilamacedo86 camilamacedo86 changed the title ✨ store all config info (go/v3-alpha) [wip] - ✨ store all config info (go/v3-alpha) Dec 3, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2020
Copy link
Contributor

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

There seems to be a mismatch of data between config.ResourceData, resource.Resource, and resource.Options. The latter two are constructed by the plugin layer, which passes a resource.Resource to the scaffold layer to be used and updated. Therefore all of these structs should have the same field subset relating to a resource as an element of config.ResourceData.Resources.

pkg/model/config/config.go Outdated Show resolved Hide resolved
pkg/model/config/config.go Outdated Show resolved Hide resolved
pkg/model/config/config.go Outdated Show resolved Hide resolved
pkg/model/config/config.go Show resolved Hide resolved
pkg/model/resource/options.go Outdated Show resolved Hide resolved
pkg/plugins/golang/v3/api.go Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 changed the title [wip] - ✨ store all config info (go/v3-alpha) ✨ store all config info (go/v3-alpha) Dec 4, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2020
@camilamacedo86 camilamacedo86 force-pushed the webhooks-store branch 3 times, most recently from b213920 to c48b189 Compare December 8, 2020 15:32
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 8, 2020
@camilamacedo86 camilamacedo86 force-pushed the webhooks-store branch 4 times, most recently from 08aa09c to 278b3a9 Compare December 8, 2020 17:17
@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-18-0

@camilamacedo86
Copy link
Member Author

/hold cancel

It has all suggested attrs in the format agreed as it is addressing all suggested made.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2020
@camilamacedo86
Copy link
Member Author

Close in favor of #1869

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project file with all user input data
6 participants