Fixed the default namespace issue #150

Bharadwajshivam28 · 2024-02-11T02:10:52Z

Currently if namespace is not passed and we run the command bin/hydrophone --cleanup then the command breaks so we can pass a default namespace it users run the command bin/hydrophone --cleanup then also the default namespace will be taken.

fixes #149

Previous behaviour-

Current behaviour-

k8s-ci-robot · 2024-02-11T02:11:01Z

Hi @Bharadwajshivam28. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

reetasingh · 2024-02-11T02:29:36Z

pkg/service/init.go

@@ -323,6 +323,12 @@ func RunE2E(clientset *kubernetes.Clientset) {
 // Cleanup removes all resources created during E2E tests.
 func Cleanup(clientset *kubernetes.Clientset) {
 	namespace := viper.GetString("namespace")
+
+	if namespace == "" {


@Bharadwajshivam28 A cleaner solution would be to reuse the ValidateArgs method https://github.com/kubernetes-sigs/hydrophone/blob/main/pkg/common/args.go#L52. its currently validating multiple things. maybe we can introduce a new method for validating namespace?

Okay works for me... let me change it @reetasingh

Bharadwajshivam28 · 2024-02-11T03:39:04Z

@reetasingh I have made some changes..
could you please review it?

reetasingh · 2024-02-11T04:46:32Z

pkg/common/args.go

+		viper.Set("namespace", DefaultNamespace)
+	}
+}
+
 func ValidateArgs() error {
 	if viper.Get("namespace") == "" {


reuse SetDefaultNamespace() here too.

reuse SetDefaultNamespace() here too.

can you please elaborate this one? sorry but i not understood it..

reetasingh · 2024-02-11T04:47:45Z

pkg/common/args.go

@@ -48,6 +48,12 @@ func PrintInfo(clientSet *kubernetes.Clientset, config *rest.Config) {
 // ValidateArgs validates the arguments passed to the program
 // and creates the output directory if it doesn't exist

+func SetDefaultNamespace() {


@Bharadwajshivam28 if possible can you add unit test for this?

@Bharadwajshivam28 if possible can you add unit test for this?

I have implemented unit test but can you please elaborate this?
I will be happy to make the changes @reetasingh

you can add the unit test for this in args_test.go

reetasingh · 2024-02-11T04:48:41Z

pkg/common/args.go

@@ -48,6 +48,12 @@ func PrintInfo(clientSet *kubernetes.Clientset, config *rest.Config) {
 // ValidateArgs validates the arguments passed to the program


above comment is for ValidateArgs() method. I think you have not added it in right place

Oh I missed it... Let me take a look at it..

Will push the changes soon..

reetasingh · 2024-02-11T04:50:04Z

pkg/service/init.go

 	namespace := viper.GetString("namespace")
+	fmt.Println("using namespace:", namespace)


use log instead of fmt see https://github.com/kubernetes-sigs/hydrophone/blob/main/pkg/common/args.go#L76 for example

reetasingh · 2024-02-13T03:12:17Z

pkg/service/init.go

@@ -322,7 +322,9 @@ func RunE2E(clientset *kubernetes.Clientset) {

 // Cleanup removes all resources created during E2E tests.
 func Cleanup(clientset *kubernetes.Clientset) {
+	common.SetDefaultNamespace()


@Bharadwajshivam28 I think we can call this from cmd/root.go before calling Cleanup

can you please make me understand this? I think you are saying to call this common.SetDefaultNamespace() from the root.go?

correct, here https://github.com/kubernetes-sigs/hydrophone/blob/main/cmd/root.go#L61

oh.. you are saying to remove the line common.SetDefaultNamespace() from init.go and call it from here ?

if cleanup { common.SetDefaultNamespace() service.Cleanup(client.ClientSet)

from root.go file

reetasingh · 2024-02-13T03:18:22Z

/ok-to-test

Signed-off-by: shivam <shivambharadwaj822@gmail.com>

Bharadwajshivam28 · 2024-02-13T18:54:43Z

@reetasingh I moved common.SetDefaultNamespace() to root.go file and tested it too...

Bharadwajshivam28 · 2024-02-13T20:21:17Z

@reetasingh Just a small review i need that for the unit test what i understand for the function SetDefaultNamespace is-

Case 1

When Namespace is not passed, will set the DefaultNamespace.
After setting Namespace will check if the current namespace matches the DefaultNamespace.

Case 2

When Namespace is already set.
When Namesapce is set then we will check that the Namespace should not be replaced with the Default Namespace. Will call the SetDefaultNamespace() function before checking that the namespace not gets replaced with DefaultNamespace.

I saw the Pre declared test in args_test.go file.

Is this way we are looking? as the function SetDefaultNamespace just checks and sets namespace.

reetasingh · 2024-02-14T02:08:55Z

@reetasingh Just a small review i need that for the unit test what i understand for the function SetDefaultNamespace is-

Case 1

When Namespace is not passed, will set the DefaultNamespace.

After setting Namespace will check if the current namespace matches the DefaultNamespace.

Case 2

When Namespace is already set.

When Namesapce is set then we will check that the Namespace should not be replaced with the Default Namespace. Will call the SetDefaultNamespace() function before checking that the namespace not gets replaced with DefaultNamespace.

I saw the Pre declared test in args_test.go file.

Is this way we are looking? as the function SetDefaultNamespace just checks and sets namespace.

@Bharadwajshivam28 Its easier to review if you write the test case itself. and yes, right for each of the test case above do viper.get("namespace") and check the value of namespace matches what is expected

reetasingh · 2024-02-14T02:10:42Z

I see the presumbit job is failing

sigs.k8s.io/hydrophone/pkg/common	0.017s
FAIL	sigs.k8s.io/hydrophone/pkg/service [build failed]
FAIL
make: *** [Makefile:25: test] Error 1

can you run make test locally to see everything passes?

Bharadwajshivam28 · 2024-02-14T02:17:01Z

I see the presumbit job is failing
sigs.k8s.io/hydrophone/pkg/common	0.017s
FAIL	sigs.k8s.io/hydrophone/pkg/service [build failed]
FAIL
make: *** [Makefile:25: test] Error 1
can you run make test locally to see everything passes?

ya let me check it

Bharadwajshivam28 · 2024-02-14T02:27:09Z

@reetasingh Just a small review i need that for the unit test what i understand for the function SetDefaultNamespace is-
Case 1

When Namespace is not passed, will set the DefaultNamespace.

After setting Namespace will check if the current namespace matches the DefaultNamespace.

Case 2

When Namespace is already set.

When Namesapce is set then we will check that the Namespace should not be replaced with the Default Namespace. Will call the SetDefaultNamespace() function before checking that the namespace not gets replaced with DefaultNamespace.

I saw the Pre declared test in args_test.go file.
Is this way we are looking? as the function SetDefaultNamespace just checks and sets namespace.

@Bharadwajshivam28 Its easier to review if you write the test case itself. and yes, right for each of the test case above do viper.get("namespace") and check the value of namespace matches what is expected

Just a question @reetasingh our default namespace is conformance which will be set if namespace is not passed and if user passes the namespace then we just need to skip the test. like we dont need to do any action if users passes the namespace we just need to take actions in when users don't pass namespace..

Like we can log the namespace in both cases

Signed-off-by: shivam <shivambharadwaj822@gmail.com>

Bharadwajshivam28 · 2024-02-14T02:36:52Z

I see the presumbit job is failing
sigs.k8s.io/hydrophone/pkg/common	0.017s
FAIL	sigs.k8s.io/hydrophone/pkg/service [build failed]
FAIL
make: *** [Makefile:25: test] Error 1
can you run make test locally to see everything passes?

Pushed the code which fixes test

rjsadow · 2024-02-14T09:39:46Z

Please run make fmt and commit to make the verify test pass.

Signed-off-by: shivam <shivambharadwaj822@gmail.com>

reetasingh · 2024-02-15T00:03:43Z

/lgtm

Bharadwajshivam28 · 2024-02-15T00:12:07Z

Is there any more changes to be done mam @reetasingh

rjsadow · 2024-02-15T00:52:06Z

/approve

k8s-ci-robot · 2024-02-15T00:52:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bharadwajshivam28, rjsadow

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~~ [rjsadow]

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

Fixed the default namespace issue

e998b16

k8s-ci-robot requested review from dims and reetasingh February 11, 2024 02:10

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 11, 2024

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 11, 2024

reetasingh reviewed Feb 11, 2024

View reviewed changes

validating namespace

eaf9eb5

reetasingh reviewed Feb 11, 2024

View reviewed changes

used log instead of fmt

9633196

reetasingh reviewed Feb 13, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 13, 2024

Moved calling of function from root.go

4edca57

Signed-off-by: shivam <shivambharadwaj822@gmail.com>

Passing mage test which was faling due to incorrect usage of Printf

7da6ba5

Signed-off-by: shivam <shivambharadwaj822@gmail.com>

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 14, 2024

formatted files to pass tests

b3b74dd

Signed-off-by: shivam <shivambharadwaj822@gmail.com>

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 14, 2024

k8s-ci-robot assigned reetasingh Feb 15, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2024

k8s-ci-robot merged commit eb138c8 into kubernetes-sigs:main Feb 15, 2024
7 checks passed

Bharadwajshivam28 deleted the default_namespace branch February 16, 2024 00:34

Bharadwajshivam28 mentioned this pull request Sep 29, 2024

REQUEST: New membership for Bharadwajshivam28 kubernetes/org#5189

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed the default namespace issue #150

Fixed the default namespace issue #150

Bharadwajshivam28 commented Feb 11, 2024

k8s-ci-robot commented Feb 11, 2024

reetasingh Feb 11, 2024 •

edited

Loading

Bharadwajshivam28 Feb 11, 2024

Bharadwajshivam28 commented Feb 11, 2024

reetasingh Feb 11, 2024

Bharadwajshivam28 Feb 11, 2024

Bharadwajshivam28 Feb 11, 2024

reetasingh Feb 11, 2024

Bharadwajshivam28 Feb 11, 2024

reetasingh Feb 13, 2024

reetasingh Feb 11, 2024

Bharadwajshivam28 Feb 11, 2024

reetasingh Feb 11, 2024

reetasingh Feb 13, 2024

Bharadwajshivam28 Feb 13, 2024

reetasingh Feb 13, 2024

Bharadwajshivam28 Feb 13, 2024 •

edited

Loading

reetasingh Feb 13, 2024

reetasingh commented Feb 13, 2024

Bharadwajshivam28 commented Feb 13, 2024 •

edited

Loading

Bharadwajshivam28 commented Feb 13, 2024 •

edited

Loading

reetasingh commented Feb 14, 2024 •

edited

Loading

reetasingh commented Feb 14, 2024 •

edited

Loading

Bharadwajshivam28 commented Feb 14, 2024

Bharadwajshivam28 commented Feb 14, 2024 •

edited

Loading

Bharadwajshivam28 commented Feb 14, 2024

rjsadow commented Feb 14, 2024

reetasingh commented Feb 15, 2024

Bharadwajshivam28 commented Feb 15, 2024

rjsadow commented Feb 15, 2024

k8s-ci-robot commented Feb 15, 2024

		@@ -48,6 +48,12 @@ func PrintInfo(clientSet kubernetes.Clientset, config rest.Config) {
		// ValidateArgs validates the arguments passed to the program

		namespace := viper.GetString("namespace")
		fmt.Println("using namespace:", namespace)

Fixed the default namespace issue #150

Fixed the default namespace issue #150

Conversation

Bharadwajshivam28 commented Feb 11, 2024

k8s-ci-robot commented Feb 11, 2024

reetasingh Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bharadwajshivam28 commented Feb 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bharadwajshivam28 Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reetasingh commented Feb 13, 2024

Bharadwajshivam28 commented Feb 13, 2024 • edited Loading

Bharadwajshivam28 commented Feb 13, 2024 • edited Loading

reetasingh commented Feb 14, 2024 • edited Loading

reetasingh commented Feb 14, 2024 • edited Loading

Bharadwajshivam28 commented Feb 14, 2024

Bharadwajshivam28 commented Feb 14, 2024 • edited Loading

Bharadwajshivam28 commented Feb 14, 2024

rjsadow commented Feb 14, 2024

reetasingh commented Feb 15, 2024

Bharadwajshivam28 commented Feb 15, 2024

rjsadow commented Feb 15, 2024

k8s-ci-robot commented Feb 15, 2024

reetasingh Feb 11, 2024 •

edited

Loading

Bharadwajshivam28 Feb 13, 2024 •

edited

Loading

Bharadwajshivam28 commented Feb 13, 2024 •

edited

Loading

Bharadwajshivam28 commented Feb 13, 2024 •

edited

Loading

reetasingh commented Feb 14, 2024 •

edited

Loading

reetasingh commented Feb 14, 2024 •

edited

Loading

Bharadwajshivam28 commented Feb 14, 2024 •

edited

Loading