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

Source params reordering with docs and samples updates #810

Merged

Conversation

gabo1208
Copy link
Contributor

@gabo1208 gabo1208 commented Jun 10, 2022

Changes

  • 🎁 Source's Spec RabbitMQ related parameters have been grouped in the RabbitMQResourcesConfig Spec
  • 🎁 Source's Spec Delivery related parameters have been grouped in the Delivery Spec
  • 🎁 Added support for ClusterReference ConnectionSecret option to the Source
  • 🧹 Updating Source's docs and samples to reflect this changes

/kind api-change documentation

Fixes #

Release Note

- BREAKING: 🎁 Source's Spec RabbitMQ related parameters have been grouped in the RabbitMQResourcesConfig Spec, Source's Spec Delivery related parameters have been grouped in the Delivery Spec
- 🎁 Added support for ClusterReference ConnectionSecret option to the Source

@knative-prow knative-prow bot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/documentation labels Jun 10, 2022
@gabo1208
Copy link
Contributor Author

/hold

@knative-prow knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 10, 2022
@gabo1208 gabo1208 force-pushed the source-params-reordering branch 3 times, most recently from 3207363 to 794a837 Compare June 12, 2022 03:32
@codecov
Copy link

codecov bot commented Jun 12, 2022

Codecov Report

Merging #810 (5c19442) into main (d3524e0) will decrease coverage by 0.56%.
The diff coverage is 63.15%.

@@            Coverage Diff             @@
##             main     #810      +/-   ##
==========================================
- Coverage   72.44%   71.88%   -0.57%     
==========================================
  Files          39       39              
  Lines        2577     2600      +23     
==========================================
+ Hits         1867     1869       +2     
- Misses        645      665      +20     
- Partials       65       66       +1     
Impacted Files Coverage Δ
pkg/apis/eventing/v1alpha1/rabbitmq_defaults.go 0.00% <0.00%> (ø)
pkg/apis/sources/v1alpha1/rabbitmq_defaults.go 0.00% <0.00%> (ø)
pkg/apis/sources/v1alpha1/rabbitmq_types.go 50.00% <ø> (ø)
pkg/rabbit/service.go 8.71% <0.00%> (-0.89%) ⬇️
pkg/reconciler/source/resources/receive_adapter.go 95.41% <81.81%> (+0.03%) ⬆️
pkg/adapter/adapter.go 58.75% <83.33%> (ø)
pkg/apis/sources/v1alpha1/rabbitmq_validation.go 84.09% <96.00%> (+12.35%) ⬆️
pkg/rabbit/exchange.go 100.00% <100.00%> (ø)
pkg/rabbit/queue.go 100.00% <100.00%> (ø)
pkg/rabbit/message.go 68.90% <0.00%> (-2.53%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3524e0...5c19442. Read the comment docs.

@gabo1208 gabo1208 force-pushed the source-params-reordering branch 6 times, most recently from 425d227 to f449139 Compare June 12, 2022 18:28
@gabo1208 gabo1208 mentioned this pull request Jun 14, 2022
3 tasks
@gabo1208
Copy link
Contributor Author

/hold cancel

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2022
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2022
…oo, this includes a refactor to reuse some helper methods for the triggers, sources and brokers. Fixed some tests and docs but still some to go in other commits
… to fix the unit tests + fixed source new parameters description yaml in config dir

recovered vhost for conformance tests
…ster references + added sources to secret generator

added secret reconciliation to source

added secret informer
…lete docs pr about the new source api will come next to this one
…bbitmqResourcesConfig specs, refactored queue type and fixed tests to go along with it, now the source also have queue type + updated codegen
…g the conenction secret + fixed sample, still some work to do on the docs
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2022
@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 14, 2022
@gabo1208 gabo1208 mentioned this pull request Jun 14, 2022
8 tasks
Copy link
Contributor

@gab-satchi gab-satchi left a comment

Choose a reason for hiding this comment

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

just some minor things

pkg/adapter/adapter.go Outdated Show resolved Hide resolved
pkg/rabbit/service.go Outdated Show resolved Hide resolved
@gab-satchi
Copy link
Contributor

another nit regarding the release notes: We can just have a single line or a few lines regarding the Source API changing. With a BREAKING: prefix. With the release notes in the last few PRs, they highlight individual things the PRs are doing and that's too detailed for release notes.

@gabo1208 gabo1208 requested a review from gab-satchi June 15, 2022 19:50
Copy link
Contributor

@gab-satchi gab-satchi left a comment

Choose a reason for hiding this comment

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

some minor things

pkg/apis/eventing/v1alpha1/rabbitmq_defaults.go Outdated Show resolved Hide resolved
pkg/apis/eventing/v1alpha1/rabbitmq_defaults.go Outdated Show resolved Hide resolved
pkg/apis/sources/v1alpha1/rabbitmq_defaults.go Outdated Show resolved Hide resolved
pkg/apis/sources/v1alpha1/rabbitmq_validation.go Outdated Show resolved Hide resolved
@gabo1208 gabo1208 requested a review from gab-satchi June 16, 2022 15:34
Copy link
Contributor

@gab-satchi gab-satchi left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold for tests

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2022
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2022
@knative-prow
Copy link

knative-prow bot commented Jun 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gab-satchi, gabo1208

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:
  • OWNERS [gab-satchi,gabo1208]

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

@gab-satchi
Copy link
Contributor

/hold cancel

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2022
@knative-prow knative-prow bot merged commit 96a65a4 into knative-extensions:main Jun 16, 2022
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/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/documentation lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants