support: nodeselector in apiserversource

[sadath-12]

Fixes #7360

Proposed Changes

Users can now configure nodeSelector via config-features by adding key-value pair in the format apiserversources.nodeselector.<key>: <value>

Pre-review Checklist

• At least 80% unit test coverage
• E2E tests for any new behavior
• Docs PR for any user-facing impact
• Spec PR for any new API feature
• Conformance test for any change to the spec

Release Note

Feature: Added the ability for users to configure `nodeSelector` when deploying `apiserversource` , Users can now configure nodeSelector via config-features by adding key-value pair in the format apiserversources.nodeselector.<key>: <value>

Docs

[knative-prow]

Hi @sadath-12. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Cali0707]

Thanks for working on this @sadath-12

/ok-to-test

[codecov]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (580dc9b) 74.56% compared to head (93369b5) 74.44%.
Report is 19 commits behind head on main.

Files	Patch %	Lines
pkg/apis/feature/features.go	75.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7584      +/-   ##
==========================================
- Coverage   74.56%   74.44%   -0.12%     
==========================================
  Files         262      262              
  Lines       14951    15077     +126     
==========================================
+ Hits        11148    11224      +76     
- Misses       3215     3247      +32     
- Partials      588      606      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sadath-12]

Thank you @Cali0707 for helping me improve this

[Cali0707]

@sadath-12 can you add some tests in pkg/reconciler/apiserversource/apiserversource_test.go to test that the apiserversource reconciles properly when the node selectors are set?

[sadath-12]

Done thanks @Cali0707

[sadath-12]

other changes are just some formatting

[sadath-12]

thanks @Cali0707 .

[sadath-12]

@Cali0707 the failing tests points to timeout problem . which I'm quite sure it might not be related my because I checked with reconciler in the tests I worked and unless nodeselector is passed anywhere there should not be any behaviour or events change anywhere

[Cali0707]

@sadath-12 can you fix your formatting?

[Cali0707]

Also, I'm curious. Do you have any ideas on how to integration test this once it is merged? Ideally we won't just be relying on a few unit tests for this feature

[sadath-12]

@sadath-12 can you fix your formatting?

I can run gofumpt and whole file will be formatted but as you reviewed last time it would become difficult to review 🙂

[sadath-12]

Also, I'm curious. Do you have any ideas on how to integration test this once it is merged? Ideally we won't just be relying on a few unit tests for this feature

I haven't tried writing e2e tests before but I think all we have to do create an apiServerSource_test.go file under test/e2e/ and create some configs and store it in featureFlags through context and then deploy a normal ApiServerSource with nodeSelector in the flag and then I would fetch the pod of that name and check through its spec the node details in which its deployed. I think its a good opportunity to start adding some ApiServerSource related e2e and cover some things . I would try to implement that after this pr is merged

[Cali0707]

I can run gofumpt and whole file will be formatted but as you reviewed last time it would become difficult to review 🙂

Just use gofmt instead of gofumpt, gofumpt is very opinionated and can lead to large changes (as we had before). gofmt will just fix the actual formatting problems we are seeing in CI (and is also what we use to check formatting problems in Knative)

[sadath-12]

Thank you @Cali0707

[Cali0707]

I haven't tried writing e2e tests before but I think all we have to do create an apiServerSource_test.go file under test/e2e/ and create some configs and store it in featureFlags through context and then deploy a normal ApiServerSource with nodeSelector in the flag and then I would fetch the pod of that name and check through its spec the node details in which its deployed. I think its a good opportunity to start adding some ApiServerSource related e2e and cover some things . I would try to implement that after this pr is merged

@sadath-12 I opened an issue here: #7615 so that we could track the work to add a test and don't forget about it.

[Cali0707]

Besides that, it looks like one of the code style jobs is still failing. Would you be able to look into that?

[sadath-12]

I haven't tried writing e2e tests before but I think all we have to do create an apiServerSource_test.go file under test/e2e/ and create some configs and store it in featureFlags through context and then deploy a normal ApiServerSource with nodeSelector in the flag and then I would fetch the pod of that name and check through its spec the node details in which its deployed. I think its a good opportunity to start adding some ApiServerSource related e2e and cover some things . I would try to implement that after this pr is merged

@sadath-12 I opened an issue here: #7615 so that we could track the work to add a test and don't forget about it.

thank you @Cali0707 I assigned it to me

[sadath-12]

Besides that, it looks like one of the code style jobs is still failing. Would you be able to look into that?

seems to be resolved now 🙂

[Cali0707]

/lgtm
/approve
@sadath-12 can you add a release note to this PR so that this makes it into the release notes for the next release? You should be able to edit your original PR description to include it.

/hold

We can unhold this once we have release notes :)

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, sadath-12

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 [Cali0707]

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

[sadath-12]

Added @Cali0707

[Cali0707]

/test upgrade-tests
/unhold