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

Change log from error to info #343

Merged

Conversation

atheo89
Copy link
Member

@atheo89 atheo89 commented Jun 6, 2024

This is a follow up enhancement to cover this discussion: #329 (comment)

Related to: https://issues.redhat.com/browse/RHOAIENG-8232

How Has This Been Tested?

Replace the odh-notebook-controller deployment with the following image: quay.io/rh_ee_atheodor/odh-notebook-controller:log-info2

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@openshift-ci openshift-ci bot requested review from harshad16 and jstourac June 6, 2024 08:38
@jstourac
Copy link
Member

jstourac commented Jun 6, 2024

Is this supposed to be a complete fix for https://issues.redhat.com/browse/RHOAIENG-8232 or is this just some preliminary partial fix until we know what all we want to do here? I am asking also whether we should put this issue in the commit message to be grabbed by errata or not.

@jiridanek
Copy link
Member

/test all

Copy link
Member

@jstourac jstourac left a comment

Choose a reason for hiding this comment

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

Since we have a followup issue to track to improve the implementation for the namespace and imagestreams lookup (https://issues.redhat.com/browse/RHOAIENG-8390), I am okay with this as is for now.

@jiridanek
Copy link
Member

let's try something

/approve

@jiridanek
Copy link
Member

/approve cancel

@harshad16
Copy link
Member

Thanks for all the review
/lgtm
/approve

Copy link

openshift-ci bot commented Jun 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16, jiridanek, jstourac

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

@openshift-merge-bot openshift-merge-bot bot merged commit 9ece6b9 into opendatahub-io:v1.7-branch Jun 17, 2024
7 checks passed
@harshad16
Copy link
Member

/cherrypick stable

@openshift-cherrypick-robot

@harshad16: new pull request created: #355

In response to this:

/cherrypick stable

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-sigs/prow repository.

@shalberd
Copy link

Since we have a followup issue to track to improve the implementation for the namespace and imagestreams lookup (https://issues.redhat.com/browse/RHOAIENG-8390), I am okay with this as is for now.

yeah, this should be an easy fix, see comment at https://issues.redhat.com/browse/RHOAIENG-8390

something akin to this (thanks, Github Copilot ...)

package main
 
import (
	"fmt"
	"os"
 
	"k8s.io/client-go/rest"
)
 
func getControllerNamespace() (string, error) {
	// Check if running inside a Kubernetes cluster
	if _, err := rest.InClusterConfig(); err != nil {
		return "", fmt.Errorf("not running inside a Kubernetes cluster")
	}
 
	// Try to get the namespace from the service account secret
	if data, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace"); err == nil {
		if ns := string(data); len(ns) > 0 {
			return ns, nil
		}
	}
 
	return "", fmt.Errorf("unable to determine the namespace")
}
 
func main() {
	namespace, err := getControllerNamespace()
	if err != nil {
		fmt.Printf("Error: %v\n", err)
		return
	}
	fmt.Printf("Controller is running in namespace: %s\n", namespace)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants