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

learning and tests #638

Closed
wants to merge 4 commits into from
Closed

learning and tests #638

wants to merge 4 commits into from

Conversation

S-ign
Copy link

@S-ign S-ign commented Feb 28, 2021

Tips:

  • please read the style guide before submitting your patch:
    docs/style-guide.md

  • commit message titles must be in the form:

topic: Capitalized message with no trailing period

or:

topic, topic2: Capitalized message with no trailing period

  • golang code must be formatted according to the standard, please run:
make gofmt		# formats the entire project correctly

or format a single golang file correctly:

gofmt -w yourcode.go
  • please rebase your patch against current git master:
git checkout master
git pull origin master
git checkout your-feature
git rebase master
git push your-remote your-feature
hub pull-request	# or submit with the github web ui
  • after a patch review, please ping @purpleidea so we know to re-review:
# make changes based on reviews...
git add -p		# add new changes
git commit --amend	# combine with existing commit
git push your-remote your-feature -f
# now ping @purpleidea in the github PR since it doesn't notify us automatically

Thanks for contributing to mgmt and welcome to the team!

// IPPort returns the combind IP:(input[0]) and Port:(input[1]) arguments
// as long as port input is within range 1-65536
func IPPort(input []types.Value) (types.Value, error) {
ip := input[0].Str()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should validate the IP address too: https://golang.org/pkg/net/#ParseIP

Copy link
Author

Choose a reason for hiding this comment

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

Oh ok, cool, didn't realize there was already an IP Parser, I'll add that! :)

return &types.StrValue{V: ""}, fmt.Errorf("err converting str to int %v", err)
}
if portInt < 1 || portInt > 65536 {
return &types.StrValue{V: ""}, errors.New("port number must be between 1-65536")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the wording here is slightly confusing given the logic. I would expect either between -0-65536 (exclusive) or preferably between 1-65535 (inclusive). The current wording is exclusive-inclusive which is probably not what the user would expect.

Side note, in many cases a port number of 0 can be valid as it's used to denote "any port". Perhaps we should allow that

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I updated this, hopefully, the wording is a little better.

// Success
{"success", "192.168.1.1", "80", "192.168.1.1:80", nil},
// Fail
{"fail", "192.168.1.1", "9483670", "", errors.New("port number must be between 1-65536")},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get some IPv6 tests too?
Also some tests for invalid IP address/port combos if you add IP address parsing

Copy link
Author

Choose a reason for hiding this comment

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

net/ParseIP made this really easy to implement, thanks!

@purpleidea
Copy link
Owner

Side note, re discussion in IRC:

I noticed that one of the tests failed in an unusual place in this PR... And I said, gah, I hope it's not a race. So I dug into it, and it turns out it was caused by a bug in your code! (yay, no our fault!) So the good news is the tests work, the bad news is I think we could have caught this issue in a more obvious place. So I added a new commit that should help new developers out by providing a clearer error message.

887d374

You'll want to rebase your branch against this commit which is now in git master.

Cheers!

@purpleidea
Copy link
Owner

@S-ign Hey-- I'm not going to do a full review yet-- I'd like you to first make a small examples/lang/whatever.mcl that uses this function, and confirm you got it doing what you expect. This should replace the copied example that you committed in this PR.

When you've succeeded, please update this PR and put a comment here with the command you used to run this example =D

Thanks!

@purpleidea
Copy link
Owner

(For anyone else reading, @S-ign came to us for a learning experience, so this is not the traditional review process.)

@S-ign S-ign closed this Mar 4, 2021
@S-ign S-ign deleted the feat/tests branch March 4, 2021 23:17
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.

3 participants