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

prepare provider for tfplugindocs #83

Merged
merged 3 commits into from Jul 26, 2022

Conversation

maksym-nazarenko
Copy link
Collaborator

Part of #82

Comment on lines +35 to +40
- `ca_certificate` (String) Path to MikroTik's certificate authority
- `host` (String) Hostname of the MikroTik router
- `insecure` (Boolean) Insecure connection does not verify MikroTik's TLS certificate
- `password` (String) Password for MikroTik api
- `tls` (Boolean) Whether to use TLS when connecting to MikroTik or not
- `username` (String) User account for MikroTik api
Copy link
Owner

Choose a reason for hiding this comment

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

username, password and host are not optional fields. So it seems like the provider's arguments aren't set correctly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's how it is declared:

"username": {
    Type:        schema.TypeString,
    Required:    true,
    DefaultFunc: schema.EnvDefaultFunc("MIKROTIK_USER", nil),
    Description: "User account for mikrotik api",
},

so yes, it should either be set to Optional or update the docs and mention that there is a fallback to env vars.

@ddelnano
Copy link
Owner

I'd like to merge this PR first so we can rebase the others to remove the shared changes. I had one question about why everything is considered optional, otherwise this looks good.

@maksym-nazarenko
Copy link
Collaborator Author

@ddelnano we can turn those fields into Required - the will be fetched from env if user does not provide them in configuration.

@ddelnano
Copy link
Owner

ddelnano commented Jul 26, 2022

I see. I didn't realize that having the env in use would make these optional. Anyway, knowing that this is good as is.

@ddelnano ddelnano merged commit b7408cf into ddelnano:master Jul 26, 2022
@ddelnano
Copy link
Owner

@Maxim-Nazarenko please rebase the remaining doc update PRs and then we can get those merged.

@maksym-nazarenko
Copy link
Collaborator Author

maksym-nazarenko commented Jul 26, 2022

@ddelnano I just relaised what you was referring to.
It was an issue in tfplugindocs which rendered fields in optional section regardless of Required: true attribute.
I'll update it in follow-up PR.

Never mind, it was my local issue: for some reason exactly that integrated terminal in VS Code was broken - tfplugindocs worked as expected everywhere except the opened terminal o_O. Had to close it and open again 🤷

Sharp eye!

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.

None yet

2 participants