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

Improvements on the resource routeros_interface_ethernet #266

Merged
merged 11 commits into from
Oct 1, 2023

Conversation

jlpedrosa
Copy link
Contributor

@jlpedrosa jlpedrosa commented Sep 26, 2023

Add examples for routeros_interface_ethernet
Add additional computed properties of the routeros_interface_ethernet resource.

Disable reading fields that are being ignored (dynamic counters), so the output from terraform plan on subsequent reads is clean.

Disable sending fields not supported by the routers where the provider is pointing to.

Add examples for routeros_interface_ethernet

Add additional computed properties of the routeros_interface_ethernet resource
routeros/resource_interface_ethernet.go Outdated Show resolved Hide resolved
routeros/resource_default_actions.go Outdated Show resolved Hide resolved
routeros/resource_interface_ethernet.go Outdated Show resolved Hide resolved
routeros/resource_interface_ethernet.go Outdated Show resolved Hide resolved
@jlpedrosa
Copy link
Contributor Author

@vaerh I open the PR to test against CI and you get a preview of the work that I am doing. But I really don't have intention to bother you, even the PR may be drastically changed in next commits. I usually signal that the PR is ready to review when I change it from Draft => Ready


// {"channel": "channel.config", "mikrotik-field-name": "schema-field-name"}
if ts, ok := s[MetaTransformSet]; ok {
transformSet = loadTransformSet(ts.Default.(string), false)
}

// "field_first", "field_second", "field_third"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vaerh this 3 lines are a bit structural change. heads up, double check if you think it makes sense.
What I am trying to do here is be able to skip fields from being passed to terraform as it throws warnings about fields being missing in the schema.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks logical and good. In my opinion it should work without errors.

@jlpedrosa jlpedrosa marked this pull request as ready for review September 28, 2023 19:47
@jlpedrosa jlpedrosa requested a review from a team as a code owner September 28, 2023 19:47
@jlpedrosa jlpedrosa changed the title WIP - Improvements on the resource routeros_interface_ethernet Improvements on the resource routeros_interface_ethernet Sep 28, 2023
@@ -170,6 +170,12 @@ var (
Description: "Layer2 Maximum transmission unit. " +
"[See](https://wiki.mikrotik.com/wiki/Maximum_Transmission_Unit_on_RouterBoards).",
}
PropL2MtuRw = &schema.Schema{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in ethernet devices l2mtu is writable (and must be written).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest not putting this attribute in the helpers, as it only occurs in this form (RW) in one resource so far.

Copy link
Collaborator

Choose a reason for hiding this comment

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

KeyL2Mtu: {
Type: schema.TypeInt,
Optional: true,
Description: "Layer2 Maximum transmission unit. " +
"See.",
}

@@ -121,7 +147,7 @@ func ResourceInterfaceEthernet() *schema.Resource {
Default: true,
Optional: true,
},
KeyL2Mtu: PropL2MtuRo,
KeyL2Mtu: PropL2MtuRw,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in ethernet devices l2mtu is writable (and must be written).

return nil
func updateSchemaWithRouterCapabilities(s map[string]*schema.Schema, item MikrotikItem) map[string]*schema.Schema {
// Dynamic schema, counters for tx_queue${number}_packets, changes from router to router, read only counters.
// Just drop them as they don't have much sense in the context of a terraform provider
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning logs by terraform are in the shape of:

╷
│ Warning: Field 'tx_queue2_packet' not found in the schema
│ 
│   with routeros_interface_ethernet.eths["test-2"],
│   on temp.tf line 33, in resource "routeros_interface_ethernet" "eths":
│   33: resource "routeros_interface_ethernet" "eths" {
│ 
│ [MikrotikResourceDataToTerraform] The field was lost during the Schema development: ▷ 'tx_queue2_packet': '0' ◁


// {"channel": "channel.config", "mikrotik-field-name": "schema-field-name"}
if ts, ok := s[MetaTransformSet]; ok {
transformSet = loadTransformSet(ts.Default.(string), false)
}

// "field_first", "field_second", "field_third"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks logical and good. In my opinion it should work without errors.

@@ -170,6 +170,12 @@ var (
Description: "Layer2 Maximum transmission unit. " +
"[See](https://wiki.mikrotik.com/wiki/Maximum_Transmission_Unit_on_RouterBoards).",
}
PropL2MtuRw = &schema.Schema{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest not putting this attribute in the helpers, as it only occurs in this form (RW) in one resource so far.

@@ -170,6 +170,12 @@ var (
Description: "Layer2 Maximum transmission unit. " +
"[See](https://wiki.mikrotik.com/wiki/Maximum_Transmission_Unit_on_RouterBoards).",
}
PropL2MtuRw = &schema.Schema{
Copy link
Collaborator

Choose a reason for hiding this comment

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

KeyL2Mtu: {
Type: schema.TypeInt,
Optional: true,
Description: "Layer2 Maximum transmission unit. " +
"See.",
}

@vaerh vaerh merged commit 099185b into terraform-routeros:main Oct 1, 2023
3 checks passed
@vaerh
Copy link
Collaborator

vaerh commented Oct 1, 2023

🎉 This PR is included in version 1.18.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@vaerh vaerh added the released label Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants