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

docs: update ipv4_cidr_block to a list #167

Merged
merged 3 commits into from
Aug 12, 2022
Merged

Conversation

morremeyer
Copy link
Contributor

@morremeyer morremeyer commented Jul 22, 2022

what

  • Updates documentation for ipv4_cidr_block

why

  • The current documentation is wrong

additional info

I tried to run make init && make readme to generate the README, however make readme fails with:

❯ make readme
* Package gomplate already installed
* Package terraform-docs already installed
make: gomplate: No such file or directory
make: *** [readme/build] Error 1

on my machine. (MacBook Pro, macOS Monterey 12.4)

@morremeyer morremeyer requested review from a team as code owners July 22, 2022 11:12
@joe-niland
Copy link
Member

/test all

@joe-niland
Copy link
Member

@morremeyer I know it's not relevant to your change but the tests failed due to a missing terraform version attribute in example/complete. Could you please copy versions.tf from the project root to the examples/complete and examples/existing-ip directories?

@morremeyer
Copy link
Contributor Author

@joe-niland Sure, no problem :)

@joe-niland
Copy link
Member

/test bats

@joe-niland
Copy link
Member

/test all

@joe-niland joe-niland added the patch A minor, backward compatible change label Aug 1, 2022
@joe-niland joe-niland enabled auto-merge (squash) August 1, 2022 10:58
@joe-niland
Copy link
Member

Thanks very much @morremeyer, looks good! I will ask one of the Code owners to review.

btw, if you feel like raising the gomplate issue.

@aknysh
Copy link
Member

aknysh commented Aug 12, 2022

/test all

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

thanks @morremeyer

@aknysh aknysh merged commit d11bd52 into cloudposse:master Aug 12, 2022
@Nuru
Copy link
Contributor

Nuru commented Aug 12, 2022

@morremeyer @joe-niland Regarding the make readme issue, make readme runs tools (such as gomplate) on the host to build the README.md file and will fail if you do not have them installed. You can, instead, run make pr/readme which will run the tools inside the build-harness Docker container and should work in more situations.

@joe-niland
Copy link
Member

joe-niland commented Aug 14, 2022

@Nuru thanks. Should make readme/deps handle the native dependency install? I remember trying this unsuccessfully and just installed via brew.

Anyway, thanks for the pointer on the docker option - that sounds preferable.

@morremeyer
Copy link
Contributor Author

I would advise against that, I wouldn't want a "generate Textfile" command to install Software without it being documented explicitly.

A make setup or similar, or just documentation about which dependencies need to be installed would be much better in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch A minor, backward compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants