-
Notifications
You must be signed in to change notification settings - Fork 62
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
Migrate action to official repository #1
Conversation
@Dirrk would you mind taking a look at this and let me know what's missing? Note that I briefly modified some of the configuration but it's still heavily WIP at this point and I wanted to have your feedback from early on. |
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.
Just commenting on this as it stands. I think removing the docker build from this repo is definitely the correct path forward. I haven't looked over everything but so far 👍
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.
Thank you @Dirrk , I have some questions below.
@@ -6,7 +6,7 @@ | |||
## Version | |||
{{ $version }} | |||
|
|||
Using [terraform-docs](https://github.com/segmentio/terraform-docs) v0.8.2, which is supported and tested on terraform version 0.11+ & 0.12+ but may work for others. | |||
Using [terraform-docs](https://github.com/terraform-docs/terraform-docs) `v0.8.2`, which is supported and tested on terraform version 0.11+ & 0.12+ but may work for others. |
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.
I'm not familiar with .github/templates/
folder, what's the purpose of this file?
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.
I was playing around with some other github actions for automating the readme/changelog and its just left over from then.
There is still a script (scripts/pre-release.sh) that can be used to run this template file to generate the main README.md
action.yml
Outdated
inputs: | ||
tf_docs_working_dir: | ||
working_dir: |
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.
As you can see I've removed the already implied tf_docs_
prefix from inputs and also I've been thinking to rename them to hyphen-separated rather than snake case. What do you think of this?
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.
I am perfectly fine with removing the tf_docs_ prefix but if you do hyphen github won't be able to pass them down as variables as I am fairly certain -
will not be a valid variable name but I could be wrong.
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.
To be specific about the variables, they get passed in as environment variables and bash doesn't support -
.
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.
I see, thanks. I've actually found a way to pass -
separated inputs down to the docker image without losing them. I'm gonna push something soon for it.
I personally prefer hyphens because they are more readable.
Signed-off-by: Khosrow Moossavi <khos2ow@gmail.com>
Description
Migrating the code from https://github.com/Dirrk/terraform-docs to here.
Fixes Dirrk/terraform-docs#24