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

add getHostByName function to resolve dns names to ips #173

Merged
merged 3 commits into from
Sep 24, 2019

Conversation

fcgravalos
Copy link

Hi guys!

This is a proposal to add a function to resolve names to IPs. I'm a heavy user of helm and this is useful where you need to render an IP but you don't want to hardcode it in your templates.

What do you think?

@fcgravalos
Copy link
Author

Any chance I get feedback on this?

Copy link
Member

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

Note, in the future if more network functions are created the file for them should be renamed.

Copy link
Member

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

Two pieces of feedback:

  1. Can you name the file network.go and network_test.go
  2. Can you update the documentation to include this function

@fcgravalos
Copy link
Author

Two pieces of feedback:

  1. Can you name the file network.go and network_test.go
  2. Can you update the documentation to include this function

Thanks for the feedback!!

Done!

docs/network.md Outdated
@@ -0,0 +1,11 @@
# Network Functions

Sprig has a number of string manipulation functions.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a copy and paste error. This is the network functions not the string ones.

Copy link
Author

Choose a reason for hiding this comment

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

sorry dude, I'll fix it

network.go Outdated
"net"
)

func getHostByName(name string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry I didn't catch this before but returning an error has some consequences. It stops execution of the template where it is at and returns an error from Execute. You can see an example with this function at https://play.golang.org/p/d1f0BridbDc.

For sprig v3 we are going to add error handling via #132 and expanding it to cover any others.

Would you mind ignoring the error for now and we'll add that back when we have a consistent setup for both with and without error handling at that level.

For what it's worth, v3 is coming this month.

Copy link
Author

@fcgravalos fcgravalos Sep 23, 2019

Choose a reason for hiding this comment

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

Sure no problem at all!! :). I'll ignore the error

Copy link
Author

Choose a reason for hiding this comment

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

I'll remove error catching from the test as well

@fcgravalos
Copy link
Author

fcgravalos commented Sep 23, 2019

@mattfarina it seems one of the build steps failed due to a connectivity issue towards git.luolix.topm can we re-trigger the job? I didn't find the way to do it

Cloning into 'Masterminds/sprig'...
fatal: unable to access 'https://github.com/Masterminds/sprig.git/': Failed to connect to github.com port 443: Connection timed out

@mattfarina
Copy link
Member

re-running that test

@mattfarina
Copy link
Member

The testing error is due to the setup target not being on master any longer. strange.

@mattfarina mattfarina merged commit 022461a into Masterminds:master Sep 24, 2019
@fcgravalos
Copy link
Author

Thanks @mattfarina!!

@fcgravalos fcgravalos deleted the get_host_by_name branch September 25, 2019 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants