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

Refactor the way Triggers creates child resources to follow Knative conventions #1205

Merged
merged 5 commits into from
Aug 27, 2021

Conversation

mattmoor
Copy link
Member

@mattmoor mattmoor commented Aug 26, 2021

Generally in Knative our controller structure follows the layout:

pkg/
   reconciler/
      {resource-name}/
          controller.go      # The *controller.Impl ctor (deals with informers and workqueues)
          {resource-name}.go # The typed reconciler (deals with listers and clients)
          resources/
              {child-resource-name}.go  # Logic for building this child resource.
              ...
              names/
                 names.go # Optionally holds methods for naming child resources.

This change starts to move things further towards this structure with a new
resources/ sub-directory (I did not start a names/ sub-directory). I have
tried to limit my changes beyond moving code around, but there are a handful
of tweaks I've made, and tests I've added. At this point this new package should
have 100% coverage, which I'll try to preserve as I continue down this path.

/kind cleanup

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

NONE

cc @dibyom @savitaashture

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 26, 2021
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 26, 2021
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/eventlistener/eventlistener.go 76.9% 75.1% -1.8
pkg/reconciler/eventlistener/resources/config.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/meta.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/service.go Do not exist 100.0%

@mattmoor mattmoor changed the title Refactor the way Triggers creates child resources to follow Knative conventions [WIP] Refactor the way Triggers creates child resources to follow Knative conventions Aug 26, 2021
@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2021
@mattmoor mattmoor force-pushed the resources-directory branch from 7094cbc to 042dc86 Compare August 26, 2021 18:29
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/eventlistener/eventlistener.go 76.9% 75.1% -1.8
pkg/reconciler/eventlistener/resources/config.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/meta.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/service.go Do not exist 100.0%

@mattmoor mattmoor force-pushed the resources-directory branch from 042dc86 to c3fd34b Compare August 26, 2021 18:43
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/eventlistener/eventlistener.go 76.9% 75.1% -1.8
pkg/reconciler/eventlistener/resources/config.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/meta.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/service.go Do not exist 100.0%

…onventions.

Generally in Knative our controller structure follows the layout:
```
pkg/
  reconciler/
    {resource-name}/
      controller.go      # The *controller.Impl ctor (deals with informers and workqueues)
      {resource-name}.go # The typed reconciler (deals with listers and clients)
      resources/
        {child-resource-name}.go  # Logic for building this child resource.
        ...
        names/
          names.go # Optionally holds methods for naming child resources.
```

The main piece of this that this change start to move things towards is the
`resources/` sub-directory (I did not start a `names/` sub-directory).
@mattmoor mattmoor force-pushed the resources-directory branch from c3fd34b to f8bae70 Compare August 26, 2021 19:07
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/eventlistener/eventlistener.go 76.9% 74.7% -2.2
pkg/reconciler/eventlistener/resources/config.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/container.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/meta.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/service.go Do not exist 100.0%

@dibyom
Copy link
Member

dibyom commented Aug 26, 2021

Nice! The folder structure makes sense!

@mattmoor
Copy link
Member Author

I've been pushing progress as I've gone. I'm working on deployment now, which is a bit more complex than the rest 😅

Should have something RFAL tonight.

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/eventlistener/eventlistener.go 76.9% 71.1% -5.8
pkg/reconciler/eventlistener/resources/config.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/container.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/deployment.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/meta.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/service.go Do not exist 100.0%

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/eventlistener/eventlistener.go 76.9% 69.8% -7.1
pkg/reconciler/eventlistener/resources/config.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/container.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/custom.go Do not exist 93.9%
pkg/reconciler/eventlistener/resources/deployment.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/meta.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/service.go Do not exist 100.0%

@mattmoor
Copy link
Member Author

Hmm, I don't see why that failed scanning the logs. Let's see if it repros before I spend too long scanning it with a fine toothed comb...

/retest

@mattmoor mattmoor force-pushed the resources-directory branch from 741bf22 to bd158bc Compare August 26, 2021 23:05
@mattmoor
Copy link
Member Author

Ok, I think I sorted out the failure. I'd removed a namespace local, which made the build fail, but it wasn't making my test builds fail because there's a global named namespace 🤦

Let's see how this one does.

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/eventlistener/eventlistener.go 76.9% 69.8% -7.1
pkg/reconciler/eventlistener/resources/config.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/container.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/custom.go Do not exist 93.9%
pkg/reconciler/eventlistener/resources/deployment.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/meta.go Do not exist 100.0%
pkg/reconciler/eventlistener/resources/service.go Do not exist 100.0%

@mattmoor mattmoor changed the title [WIP] Refactor the way Triggers creates child resources to follow Knative conventions Refactor the way Triggers creates child resources to follow Knative conventions Aug 26, 2021
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2021
@dibyom
Copy link
Member

dibyom commented Aug 27, 2021

/approve
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2021
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2021
@tekton-robot tekton-robot merged commit 2aafa87 into tektoncd:main Aug 27, 2021
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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. 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.

3 participants