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.

Sign up for GitHub

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

Jump to bottom

Added function getEnvVar() #196

Merged
merged 4 commits into from
Jul 9, 2024
Merged

Added function getEnvVar() #196

merged 4 commits into from
Jul 9, 2024

Conversation

jaredoconnell
Copy link
Contributor

Changes introduced with this PR

This is a simple function to get en env variable. This has many potential uses, including getting the kubeconfig location from the KUBECONFIG environment variable.

By contributing to this repository, I agree to the contribution guidelines.

dbutenhof
dbutenhof reviewed Jul 8, 2024
View reviewed changes
Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

Looks good, although I'm not thrilled about the function name and you're also missed a reference.

internal/builtinfunctions/functions.go Outdated Show resolved Hide resolved
internal/builtinfunctions/functions.go Outdated Show resolved Hide resolved
webbnh
webbnh approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

I second Dave's comments.

internal/builtinfunctions/functions.go Outdated Show resolved Hide resolved
dbutenhof
dbutenhof approved these changes Jul 9, 2024
View reviewed changes
internal/builtinfunctions/functions_test.go Outdated Show resolved Hide resolved
@dbutenhof dbutenhof changed the title Added function readEnvVar() Added function getEnvVar() Jul 9, 2024
dbutenhof
dbutenhof approved these changes Jul 9, 2024
View reviewed changes
Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

I still think it's weird that a change like this doesn't invalidate my previous ✅ ... but I'll give you another ✅ anyway for your collection. 😆

webbnh
webbnh approved these changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

I still think it's weird that a change like this doesn't invalidate my previous ✅ .

You should talk to the repo owner about getting that setting fixed. 😆

@jaredoconnell jaredoconnell merged commit ee454cc into main Jul 9, 2024
5 checks passed
@jaredoconnell jaredoconnell deleted the env-var-function branch July 9, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webbnh webbnh webbnh approved these changes

@dbutenhof dbutenhof dbutenhof approved these changes

@dustinblack dustinblack Awaiting requested review from dustinblack

@mfleader mfleader Awaiting requested review from mfleader

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jaredoconnell @webbnh @dbutenhof