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

Rule.String() method returns .Dst #688

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Rule.String() method returns .Dst #688

merged 1 commit into from
Sep 20, 2021

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Aug 26, 2021

  • Return to <all | ipNetString> in string
  • Return all for from if rule.Src == nil
  • Return all for to if rule.Dst == nil

@aboch
Copy link
Collaborator

aboch commented Sep 17, 2021

pls rebase and force push so that CI (now fixed) will be triggered

@qdm12
Copy link
Contributor Author

qdm12 commented Sep 20, 2021

Done 👍 I also added a unit test to cover most cases I can think of.

My editor keep on adding //go:build linux on save (because Go 1.17) but that shouldn't cause any issue with previous Go versions. Let me know if this bothers you.

@aboch
Copy link
Collaborator

aboch commented Sep 20, 2021

Please squash your commits into one then force push. (This repo does not have "Squash-Merge" option enabled)

- Return `to <all | ipNetString>` in string
- Return `all` for `from` if `rule.Src == nil`
- Return `all` for `to` if `rule.Dst == nil`

Add unit test
@qdm12
Copy link
Contributor Author

qdm12 commented Sep 20, 2021

Please squash your commits into one then force push.

Done 😉

This repo does not have "Squash-Merge" option enabled

Why not out of curiosity 🤔 ?

@aboch
Copy link
Collaborator

aboch commented Sep 20, 2021

Why not out of curiosity thinking ?

It was like this since I came around here.

I think Vish wanted to have a clear 1-to-1 mapping between commit and PR. Which I also like.

@aboch aboch merged commit 30ec08b into vishvananda:master Sep 20, 2021
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