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

Update documentation #5

Merged
merged 5 commits into from
Sep 6, 2017
Merged

Update documentation #5

merged 5 commits into from
Sep 6, 2017

Conversation

drama17
Copy link
Contributor

@drama17 drama17 commented Aug 24, 2017

What

  • Update README.md

Why

@drama17 drama17 self-assigned this Aug 24, 2017
@drama17 drama17 requested a review from const-bon August 24, 2017 11:44
README.md Outdated
@@ -24,19 +24,30 @@ module "admin_tier" {
stage = "${var.stage}"
subnets = ["${var.subnets}"]
zone_id = "${module.tf_domain.zone_id}"
security_groups = ["${var.security_groups}"]
associate_public_ip_address = "${var.associate_public_ip_address}"
}

```

This will create a `id`, `fqdn`, `security_group_id`, `role` and `public_ip`.
Copy link
Contributor

Choose a reason for hiding this comment

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

id, fqdn, security_group_id, role and public_ip are outputs, but not created resources.
Delete it, as we have tf_admin purpose description at the top and outputs description at the bottom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

README.md Outdated

`tf_admin` uses `tf_hostname` to create a DNS record for created host. `tf_hostname` module needs `zone_id` parameter as an input, and this parameter actually is an output from `tf_domain`

That is why `tf_domain` should be implemented in `root` module when we need `tf_admin`
Copy link
Contributor

Choose a reason for hiding this comment

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

That is why tf_domain should be implemented in root TF manifest when we need tf_admin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not agree, need to discuss

Copy link
Contributor

Choose a reason for hiding this comment

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

What are you against of?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

README.md Outdated
@@ -67,17 +78,18 @@ resource "aws_ami_from_instance" "example" {
| `security_groups` | [] | List of Security Group IDs allowed to connect to creating instance | Yes |
| `subnets` | [] | List of VPC Subnet IDs creating instance launched in | Yes |
| `zone_id` | `` | ID of the domain zone to use - is a result of tf_domain output | Yes |
| `associate_public_ip_address`| `true` | Define if the `public_ip` will be created for the instance | No |
Copy link
Contributor

Choose a reason for hiding this comment

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

Get description of associate_public_ip_address in tf_instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@comeanother please add more details what do you need me to change here

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Navigate to tf_instance
  2. Copy description of associate_public_ip_address in tf_instance
  3. Paste it into tf_admin associate_public_ip_address description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

README.md Outdated
| Name | Decription |
|:-------------------:|:-----------------------------------------------------------------:|
| `id` | Disambiguated ID |
| `fqdn` | Normalized name |
Copy link
Contributor

Choose a reason for hiding this comment

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

fqdn is not only Normalized name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@comeanother So you mean I need to decrypt fqdn as 'Fully Qualified Domain Name'?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's say DNS name (Fully Qualified Domain Name) of creating instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

README.md Outdated
|:-------------------:|:-----------------------------------------------------------------:|
| `id` | Disambiguated ID |
| `fqdn` | Normalized name |
| `public_ip` | Normalized namespace |
Copy link
Contributor

Choose a reason for hiding this comment

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

public_ip is not a Normalized namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course not. Fixed

README.md Outdated

`tf_admin` uses `tf_hostname` to create a DNS record for created host. `tf_hostname` module needs `zone_id` parameter as an input, and this parameter actually is an output from `tf_domain`

That is why `tf_domain` should be implemented in `root` TF manifest when we need `tf_admin`
Copy link
Member

Choose a reason for hiding this comment

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

Missing ending .

README.md Outdated

This module depends on these modules:
Module `tf_admin` requires another module to be used additionally - `tf_domain`
Copy link
Member

Choose a reason for hiding this comment

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

Missing ending .

README.md Outdated
This module depends on these modules:
Module `tf_admin` requires another module to be used additionally - `tf_domain`

`tf_admin` uses `tf_hostname` to create a DNS record for created host. `tf_hostname` module needs `zone_id` parameter as an input, and this parameter actually is an output from `tf_domain`
Copy link
Member

Choose a reason for hiding this comment

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

missing ending .

README.md Outdated
| `public_ip` | Normalized namespace |
| `ssh_key_pair` | Name of used AWS SSH key|
| Name | Decription |
|:-------------------:|:-----------------------------------------------------------------:|
Copy link
Member

@osterman osterman Aug 24, 2017

Choose a reason for hiding this comment

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

I prefer :---- to :----:. Use :---: for default values and required. Use :---- for everything else.

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

This row is still wrong.

@const-bon const-bon mentioned this pull request Sep 6, 2017
@const-bon const-bon merged commit 94cc1d1 into master Sep 6, 2017
@const-bon const-bon deleted the readme_v2 branch September 6, 2017 17:35
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