Skip to content

Conversation

@kassianh
Copy link
Contributor

Recreated README with terradoc, checked documentation for errors or inconsistencies.

@kassianh kassianh marked this pull request as ready for review January 10, 2022 21:54
@kassianh kassianh requested review from a team and mariux as code owners January 10, 2022 21:54
default = false
}

variable "auto_init" {
Copy link
Member

Choose a reason for hiding this comment

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

Those kind of code changes require adjustments in the main.tf also since we're checking for null there. Also, it should be added to the Changelog.md - may I ask you why we decided to implement those changes?

Copy link
Contributor Author

@kassianh kassianh Jan 12, 2022

Choose a reason for hiding this comment

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

@soerenmartius The documentation consistently put false where variables.tf had null. It looked like a mistake to me, like someone forgot to add the defaults in variables.tf. But it seems it was the other way around and the documentation was wrong. I will change it back and change the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

yeah that seems to be the case! Sorry for the confusion - this was obviously already faulty in the first place! Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I had a look at main.tf and it sets the defaults there. Now I get why it's like that.

Copy link
Member

Choose a reason for hiding this comment

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

oh that makes sense :) so eventually I was wrong also =D Good catch @kassianh

The question is how shall we handle this in the docs, @mariux ?

Copy link
Contributor Author

@kassianh kassianh Jan 12, 2022

Choose a reason for hiding this comment

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

@soerenmartius Actually the documentation is correct since if no default is set it will default to whatever the documentation says. It just does so in main.tf which I did not realize.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense! Thanks for the clarification!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eg:
In main.tf:
has_wiki = var.has_wiki == null ? lookup(var.defaults, "has_wiki", false) : var.has_wiki

And in the documentation:
variable "has_wiki" { type = bool default = false description = <<-END Set to true to enable the GitHub Wiki features on the repository. END }

So it does set it to false if it can't find has_wiki in var.defaults, hence the documentation is correct.

@kassianh kassianh dismissed soerenmartius’s stale review January 12, 2022 01:51

Reverted defaults in latest commit.

Copy link
Member

@soerenmartius soerenmartius left a comment

Choose a reason for hiding this comment

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

Very good job! Just a minor hint for the wrong version in the Readme. Would be great if @mariux could provide a second review.

Co-authored-by: Sören Martius <soeren.martius@gmail.com>
soerenmartius
soerenmartius previously approved these changes Jan 12, 2022
Copy link
Member

@soerenmartius soerenmartius left a comment

Choose a reason for hiding this comment

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

lgtm! good job!

@kassianh
Copy link
Contributor Author

Sorry, i hadn't run terradoc yet to update the readme

@kassianh
Copy link
Contributor Author

Any idea why the checks are failing @soerenmartius ?

@mariux mariux merged commit 32c1e63 into main Jan 12, 2022
@mariux mariux deleted the kassianh/feat/terradoc branch January 12, 2022 23:51
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.

4 participants