Add the frame_types argument to bridge ports #135
Conversation
There was a problem hiding this comment.
Hi @MagicMicky , thanks for your contribution!
Please, see the comments and don't hesitate to call for help if needed
| Interface string `mikrotik:"interface"` | ||
| PVId int `mikrotik:"pvid"` | ||
| Comment string `mikrotik:"comment"` | ||
| FrameTypes string `mikrotik:"frame-types"` |
There was a problem hiding this comment.
This field has value admit-all by default, but expected value does not populate it and TestBridgePort_basic fails.
There was a problem hiding this comment.
This should be updated, but I wasn't able to properly test unfortunately as I don't have a test environment ready to be used
There was a problem hiding this comment.
GithubActions will indicate success/failure once all conflicts are resolved.
| @@ -46,6 +46,12 @@ func resourceBridgePort() *schema.Resource { | |||
| Optional: true, | |||
| Description: "Short description for this association.", | |||
| }, | |||
| "frame_types": { | |||
There was a problem hiding this comment.
could you also add a validation function to allow only pre-defined values?
You can find more in the official docs
There was a problem hiding this comment.
I've tried to implement but there's probably some improvement that could be discussed. I've used an inline function and used the official slices package to simplify implementation, not sure if it's something that matches your ways of working or not! Let me know. Tested it quickly and it seems to work as expected
There was a problem hiding this comment.
we could use a standard validation package provided by the Terraform plugin SDK so we don't bring new dependencies that are not really necessary.
You can find an example in resource_bridge_port - there is a StringInSlice() validation function.
| @@ -16,13 +16,17 @@ github.com/ProtonMail/go-crypto v0.0.0-20210428141323-04723f9f07d7 h1:YoJbenK9C6 | |||
| github.com/ProtonMail/go-crypto v0.0.0-20210428141323-04723f9f07d7/go.mod h1:z4/9nQmJSSwwds7ejkxaJwO37dru3geImFUdJlaLzQo= | |||
| github.com/acomagu/bufpipe v1.0.3 h1:fxAGrHZTgQ9w5QqVItgzwj235/uYZYgbXitB+dLupOk= | |||
| github.com/acomagu/bufpipe v1.0.3/go.mod h1:mxdxdup/WdsKVreO5GpW4+M/1CE2sMG4jeGJ2sYmHc4= | |||
| github.com/agext/levenshtein v1.2.1/go.mod h1:JEDfjyjHDjOF/1e4FlBE/PkbqA9OfWu2ki2W0IB5558= | |||
There was a problem hiding this comment.
is the a reason this files changed?
I don't see changes in packages/imports 🤔
There was a problem hiding this comment.
when building it auto-updated, I've always found go.sum a bit weird. It's now updated I belive with an additional import.
Let me know your thoughts and I can have a look at solving conflicts there.
There was a problem hiding this comment.
Could you run go mod tidy before committing - it removes not used hashsums and should cleanup this file a bit.
There was a problem hiding this comment.
@MagicMicky left few minor comments, otherwise lgtm.
Could also merge/rebase your branch to match current master?
| @@ -46,6 +46,12 @@ func resourceBridgePort() *schema.Resource { | |||
| Optional: true, | |||
| Description: "Short description for this association.", | |||
| }, | |||
| "frame_types": { | |||
There was a problem hiding this comment.
we could use a standard validation package provided by the Terraform plugin SDK so we don't bring new dependencies that are not really necessary.
You can find an example in resource_bridge_port - there is a StringInSlice() validation function.
| Interface string `mikrotik:"interface"` | ||
| PVId int `mikrotik:"pvid"` | ||
| Comment string `mikrotik:"comment"` | ||
| FrameTypes string `mikrotik:"frame-types"` |
There was a problem hiding this comment.
GithubActions will indicate success/failure once all conflicts are resolved.
| @@ -16,13 +16,17 @@ github.com/ProtonMail/go-crypto v0.0.0-20210428141323-04723f9f07d7 h1:YoJbenK9C6 | |||
| github.com/ProtonMail/go-crypto v0.0.0-20210428141323-04723f9f07d7/go.mod h1:z4/9nQmJSSwwds7ejkxaJwO37dru3geImFUdJlaLzQo= | |||
| github.com/acomagu/bufpipe v1.0.3 h1:fxAGrHZTgQ9w5QqVItgzwj235/uYZYgbXitB+dLupOk= | |||
| github.com/acomagu/bufpipe v1.0.3/go.mod h1:mxdxdup/WdsKVreO5GpW4+M/1CE2sMG4jeGJ2sYmHc4= | |||
| github.com/agext/levenshtein v1.2.1/go.mod h1:JEDfjyjHDjOF/1e4FlBE/PkbqA9OfWu2ki2W0IB5558= | |||
There was a problem hiding this comment.
Could you run go mod tidy before committing - it removes not used hashsums and should cleanup this file a bit.
Very small commit from something I needed, I added the FramesTypes argument for Bridge ports
Let me know if I need to do anything more to get it merged upstream.