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

Use -n option for in-cluster rest config #1254

Merged
merged 6 commits into from
Nov 12, 2024

Conversation

tpantelis
Copy link
Contributor

This PR first adds unit tests for the restconfig module along with some minor supporting changes to the production code.

Fixes #324

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr1254/tpantelis/in-cluster-ns
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

internal/restconfig/producer_test.go Outdated Show resolved Hide resolved
internal/restconfig/producer_test.go Outdated Show resolved Hide resolved
internal/restconfig/producer_test.go Outdated Show resolved Hide resolved
internal/restconfig/producer_test.go Show resolved Hide resolved

It("should return false", func() {
wasRun, err := t.producer.RunOnSelectedPrefixedContext(prefix, func(_ *cluster.Info, _ string, _ reporter.Interface) error {
return nil
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
return nil
t.actualProcessed++
return nil

Copy link
Member

Choose a reason for hiding this comment

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

Was this supposed to be skipped?

Copy link
Contributor Author

@tpantelis tpantelis Nov 12, 2024

Choose a reason for hiding this comment

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

I assume you mean skipped wrt using noopPerContext - no I missed thus one.

internal/restconfig/producer_test.go Outdated Show resolved Hide resolved
internal/restconfig/restconfig.go Show resolved Hide resolved
@skitt
Copy link
Member

skitt commented Nov 6, 2024

For some bizarre reason GitHub won’t let me request changes, just submit review comments.

Signed-off-by: Tom Pantelis <[email protected]>
...to provide a hook for unit tests.

Signed-off-by: Tom Pantelis <[email protected]>
...so the prefixed "namespace" flag is added.

Signed-off-by: Tom Pantelis <[email protected]>
...that creates fake client sets for unit tests.

Signed-off-by: Tom Pantelis <[email protected]>
Also refactored the production code a little to increase branch
coverage.

Signed-off-by: Tom Pantelis <[email protected]>
Copy link
Member

@dfarrell07 dfarrell07 left a comment

Choose a reason for hiding this comment

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

Good one to learn from, thanks @tpantelis!

@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Nov 12, 2024
@tpantelis tpantelis merged commit 0ddd6df into submariner-io:devel Nov 12, 2024
33 checks passed
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr1254/tpantelis/in-cluster-ns]

@tpantelis tpantelis deleted the in-cluster-ns branch December 11, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-test When a PR is ready for full E2E testing
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

In-cluster subctl should take the -n option into account
4 participants