feat: modified spinner code to disable when running in CI or Github a… #215
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
pkg/common/spinner.go
Outdated
| @@ -55,7 +56,16 @@ func NewSpinner(w io.Writer) *Spinner { | |||
| } | |||
| } | |||
|
|
|||
| func isCIEnvironment() bool { | |||
| return os.Getenv("CI") != "" || os.Getenv("GITHUB_ACTIONS") != "" | |||
There was a problem hiding this comment.
is "CI" a standard environment variable?
if not, we should be using some sort of project specific prefix, to avoid conflicting with some unrelated meaning.
there are standard environment variables about the capability of the terminal, see the linked code in #210
There was a problem hiding this comment.
is "CI" a standard environment variable?
if not, we should be using some sort of project specific prefix, to avoid conflicting with some unrelated meaning.
there are standard environment variables about the capability of the terminal, see the linked code in #210
The thing is that the CI also detects the ci environment as I linked one tool in which I use the same way there and it works totally good
There was a problem hiding this comment.
I don't think we should invent our own unprefixed environment variables, this is a global namespace.
There was a problem hiding this comment.
I don't think we should invent our own unprefixed environment variables, this is a global namespace.
Can you just tell me where I can find environment variables to identify ci in Hydrophone?
There was a problem hiding this comment.
I think a more universal approach would be to identify if we're running in a terminal or not. If not, lets disable the spinner. x/term has a IsTerminal method that we may be able to make use of. I've never used it directly but this example code seems to be promising from a few minutes of testing. https://go.dev/play/p/dCeTgh5teta. If we return from Spinner.Start if IsTerminal is false that may be a good enough stop gap.
There was a problem hiding this comment.
Yeah, we check for tty in https://github.com/kubernetes-sigs/kind/blob/main/pkg/internal/env/term.go
there is not a universal environment variable for "is CI" and we shouldn't attempt to invent it here
There was a problem hiding this comment.
Some CI use a fake tty, and there are other user signals that they would like to disable this, each check is commented above.
There was a problem hiding this comment.
Hey @rjsadow @BenTheElder I have modified the spinner function and used x/term and IsTerminal to check if running in non terminal environment the spinner function should not start
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
…ctions
fixes: #210