-
Notifications
You must be signed in to change notification settings - Fork 69
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
generate: move configuration to flags #95
Conversation
Found this helpful for my use case. Had to resolve conflicts with recent In case it helps getting this PR merged, you find the rebased version of the PR branch at https://github.com/dominik-lekse/terraform-plugin-docs/tree/config-as-flags |
thanks @dominik-lekse; i've cherry picked your changes into this branch so if we get some eyes on this, we can merge it in. appreciate it. |
@detro i've just confirmed this is still working with the latest patches and merges. is this something i can get on someone's radar for review? if Hashicorp still isn't reviewing changes, that's fine and if that is the case, I'll stop spending my effort on keeping this in sync and just go a full fork instead. |
Hey @jacobbednarz , sorry for the delay. I have just taken a quick look and I think I like where this is going. But given the current state of testing in this repo, I might need to manually run some example tests to verify the current behaviour is not broken. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I think this is looking good. I wish we had time to stop and write some proper regression testing, so that contributions could be more easily handled.
This "parametrization" is very welcome, so thank you for taking the time and the care.
Before we can proceed, could you please add to the CHANGELOG
a section for 0.9.0
and a NEW FEATURES
entry, describing your work succinctly?
thanks @detro; I've gone ahead and added the CHANGELOG entry for this change. totally agree on the testing really lacking here as i had the same issue locally. i didn't end up pursuing it as the testing story in this repository is largely unit test based and this would require more integration in parts which i wasn't sure on the approach we'd like here. happy to go back and retrofit some tests onto that if we have a path forward though. |
This updates all the directory and provider configuration that was previously hardcoded to now be controlled by flags. Closes hashicorp#92
Co-authored-by: Ivan De Marino <[email protected]>
Rebasing this was painful :( Personal advice: never merge from |
👏👏👏 thanks for getting this one over the line @detro, you rock! |
the PR (hashicorp/terraform-plugin-docs#95) for the customisation we needed landed so we can go back to the mainline version now.
the PR (hashicorp/terraform-plugin-docs#95) for the customisation we needed landed so we can go back to the mainline version now.
the PR (hashicorp/terraform-plugin-docs#95) for the customisation we needed landed so we can go back to the mainline version now.
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This updates all the directory and provider configuration that was
previously hardcoded to now be controlled by flags.
Majority of the flags are self explanatory (however I would like some help on
wordsmithing better descriptions) but one that stands out is the difference
between
provider-name
andrendered-provider-name
. Within the code, thesewere previously the same value however I have a use case where the provider is
registered as "cloudflare" but we want the documentation to capitalise the "C".
I didn't make it as naive as "just capitalise the provider name" as I know folks
like GitHub who would hate having "Github" 😉 So,
provider-name
is usedwhen looking for the binary and schema and
rendered-provider-name
for when weare outputting it to the CLI or documentation. If
rendered-provider-name
isempty, then we just assume
provider-name
is fine to output.Closes #92