Tag AWS resources created with Terraform (#1552) #1701

natanlao · 2020-04-13T17:31:05Z

Author (check before every review)

Primary reviewer (before pushing merge commit)

Checked reindex label and r commit title tag
Rebased and squashed branch
Sanity-checked history
Build passes in sandbox _{or added no sandbox label}
Reindexed sandbox _{or this PR does not require reindexing}
Added PR reference to merge commit
Moved linked issue to Merged
Pushed merge commit to Github

Primary reviewer (after pushing merge commit)

Moved freebies to Merged column _{or there are no freebies in this PR}
Shortened chain _{or this PR is not the base of another PR}
Verified that N reviews labelling is accurate
Commented on demo expectations _{or labeled as no demo}
Pushed merge commit to Gitlab _{or this changes can be pushed later, together with another PR}
Reindexed dev _{or this PR does not require reindexing}
Deleted PR branch from Github and Gitlab
Unassign reviewer from PR

Amend Terraform configuration to tag all AWS resources with some
Azul-specific tags (Owner, AzulDeployment, Creator, Name) and some tags that
are specified in DCP RFC 0006 (project, env, owner, service, Name,
managedBy).

Update documentation in README.md to specify that manually-created resources
should be tagged in a manner consistent with those maintained by Terraform.

If all tags were static, it would make sense to contain tags in the shared
Terraform configuration. Because the content of the tags depends on user config
it is reasonable to maintain helper code in azul.deployment.

codecov · 2020-04-13T19:18:54Z

Codecov Report

Merging #1701 (60ed45f) into develop (42f6f7c) will decrease coverage by 0.01%.
The diff coverage is 83.08%.

@@             Coverage Diff             @@
##           develop    #1701      +/-   ##
===========================================
- Coverage    85.11%   85.10%   -0.02%     
===========================================
  Files          123      125       +2     
  Lines        12636    12822     +186     
===========================================
+ Hits         10755    10912     +157     
- Misses        1881     1910      +29

Impacted Files	Coverage Δ
src/azul/deployment.py	`70.55% <ø> (+0.26%)`	⬆️
src/azul/__init__.py	`81.80% <70.58%> (-0.14%)`	⬇️
src/azul/terraform.py	`76.10% <76.10%> (ø)`
test/test_tagging.py	`97.01% <97.01%> (ø)`
src/azul/modules.py	`100.00% <100.00%> (ø)`
test/test_doctests.py	`95.55% <100.00%> (+0.10%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42f6f7c...60ed45f. Read the comment docs.

coveralls · 2020-04-13T19:19:08Z

Pull Request Test Coverage Report for Build 8318

11 of 18 (61.11%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.03%) to 83.117%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/azul/deployment.py	10	17	58.82%

Totals
Change from base Build 8283:	-0.03%
Covered Lines:	9113
Relevant Lines:	10964

💛 - Coveralls

jessebrennan

I have some questions and suggestions on resource names:

Do the names need to be unique across resource types? I don't think it's necessary. For example, I don't think it's ambiguous to have an EC2 instance called Jesse and also an S3 bucket with Name as Jesse.
I think using proper capitalization and spaces instead of _ or - is more appropriate since this is a "human readable name"
I think it would make more sense to change the signature of get_aws_tags to
```
def get_aws_tags(resource_name: str, **kwargs) -> dict` 
```
and if you need a config.qualified_resource_name, you can do it at the call site of get_aws_tags.

src/azul/deployment.py

terraform/api_gateway.tf.json.template.py

terraform/cloudwatch.tf.json.template.py

README.md

natanlao · 2020-05-04T19:06:31Z

Some notes on this latest revision:

The name tag is kept consistent with any existing resource name, even if it's not unique. (See terraform/gitlab/gitlab.tf.json.template.py.) This can cause some irregularity with the component tag -- I am not sure if the approach I've taken is the best one.
The scope of this ticket was expanded to also tag GCP resources, but I found no GCP resources in terraform/ that supported tagging.

jessebrennan

Nice! this is coming together.

It still seems kind of weird to me to have the two distinct ways of specifying the name tag (either explicitly, or via **kwargs). For example what happens if resource_name is not specified and neither is the name kwarg passed in explicitly? I think it could be more elegant to consolidate those two code paths, but I'm pretty sure that doing it this way was Hannes's specification, in which case it should be left as it is.

I'm also uncertain about the values specified when overwriting name directly (via kwargs). I don't really see a pattern for how the names are determined nor what they're modelled after. I think it would be useful to make the naming conventions clear so that when future resources are added, the tagging stays consistent.

src/azul/deployment.py

terraform/api_gateway.tf.json.template.py

natanlao · 2020-05-13T16:52:23Z

For example what happens if resource_name is not specified and neither is the name kwarg passed in explicitly?

~~This is a good point -- I can add a check for this.~~ This actually probably isn't necessary, given the changes I've made from your (correct) point about calls to config.qualified_resource_name being moved to the call site of get_cloud_tags

natanlao · 2020-05-13T22:32:22Z

Okay, new revision! I've made the following changes:

get_cloud_tags no longer calls config.qualified_resource_name directly. Hannes's concern was minimizing effort on the part of the caller – that being said, counting invocations of get_cloud_tags, calling qualified_resource_name appears now to be the exception (about 1/3 of cases) rather than the rule (the remaining 2/3).
Updated the docstring of get_cloud_tags with guidance as to how the name tag is chosen ~~and added doctests~~
Split changes to _sanitize_tf into _fix_name_tag
Reviewed choices for the name tag across all resources. For the most part, I think those values are reasonable, with the potential exception of what's in terraform/s3.tf.json.template.py

jessebrennan

See the one change requested, but otherwise I approve.

src/azul/deployment.py

hannes-ucsc · 2020-11-16T21:48:39Z

scripts/prepare_lambda_deployment.py

-            for oldref, newref in replace.items():
-                value = value.replace(oldref, newref)
-            return value
+            pattern = re.compile(r'\${(\w+\.\w+)(?:-event)(\.\w+)}')


You're missing the point of re.compile. If it is used, it should be in a scope more long-lived than that of the usages of its result. So either do that or don't use it at all.

I dislike how the name translation is coded twice, once as a string operation in _patch_name and as regex replacement here. Change the regex to match a TF reference and send the matches through _patch_name.

hannes-ucsc · 2020-11-16T21:59:57Z

src/azul/deployment.py

+    def tracked_schema(self) -> JSON:
+        return self._tracked_schema_json()['schema']
+
+    def check_tracked_versions(self, versions: str) -> bool:


Not sure why we need a method for a simple == comparison. The comparison is NOT an implementation detail that should abstracted away, nor does this method reduce code or duplication. It only has one effect: obfuscation. Inline and remove.

… because no providers are installed.

hannes-ucsc · 2020-12-11T07:19:05Z

For demo, show labelled resources.

natanlao requested a review from jessebrennan April 13, 2020 17:31

natanlao self-assigned this Apr 13, 2020

github-actions bot added the orange [process] Done by the Azul team label Apr 13, 2020

hannes-ucsc unassigned natanlao Apr 13, 2020

natanlao force-pushed the issues/natanlao/1552-tag-azul-resources branch from d0fe56a to c96df34 Compare April 13, 2020 19:01

natanlao assigned jessebrennan Apr 13, 2020

jessebrennan suggested changes Apr 13, 2020

View reviewed changes

jessebrennan removed their assignment Apr 13, 2020

natanlao mentioned this pull request Apr 20, 2020

Tag Azul resources #1552

Closed

natanlao force-pushed the issues/natanlao/1552-tag-azul-resources branch 3 times, most recently from 21423f0 to 54f501a Compare May 4, 2020 18:49

natanlao requested a review from jessebrennan May 4, 2020 19:06

jessebrennan suggested changes May 4, 2020

View reviewed changes

src/azul/deployment.py Outdated Show resolved Hide resolved

src/azul/deployment.py Outdated Show resolved Hide resolved

src/azul/deployment.py Outdated Show resolved Hide resolved

terraform/api_gateway.tf.json.template.py Outdated Show resolved Hide resolved

natanlao force-pushed the issues/natanlao/1552-tag-azul-resources branch 3 times, most recently from 5093131 to 13c73b6 Compare May 13, 2020 22:12

natanlao force-pushed the issues/natanlao/1552-tag-azul-resources branch from 13c73b6 to 524695e Compare May 13, 2020 22:42

natanlao requested a review from jessebrennan May 13, 2020 23:03

natanlao assigned jessebrennan May 13, 2020

jessebrennan approved these changes May 13, 2020

View reviewed changes

src/azul/deployment.py Outdated Show resolved Hide resolved

jessebrennan removed their assignment May 14, 2020

natanlao force-pushed the issues/natanlao/1552-tag-azul-resources branch 3 times, most recently from 9dcf35d to 421a914 Compare May 18, 2020 17:01

natanlao requested a review from hannes-ucsc November 16, 2020 19:50

natanlao assigned hannes-ucsc Nov 16, 2020

hannes-ucsc requested changes Nov 16, 2020

View reviewed changes

hannes-ucsc removed their assignment Nov 16, 2020

natanlao force-pushed the issues/natanlao/1552-tag-azul-resources branch from 6211483 to c492344 Compare November 19, 2020 21:43

natanlao requested a review from hannes-ucsc November 19, 2020 22:22

theathorn assigned hannes-ucsc Nov 20, 2020

hannes-ucsc force-pushed the issues/natanlao/1552-tag-azul-resources branch 3 times, most recently from f4de671 to 9275baa Compare December 8, 2020 00:35

natanlao and others added 4 commits December 10, 2020 16:17

Refactor script doctests to call load_script

d80b5d1

[1/3] Tag AWS resources created with Terraform (#1552)

33f1e16

[2/3] Tag AWS resources created with Terraform (#1552)

11f1ea0

Fix PyCharm type warning on es_endpoint_env

44d20e3

hannes-ucsc force-pushed the issues/natanlao/1552-tag-azul-resources branch 4 times, most recently from 62bd0b6 to 6555ddb Compare December 11, 2020 05:43

hannes-ucsc added 4 commits December 10, 2020 22:32

[3/3] Tag AWS resources created with Terraform (#1552)

27cf75e

Collect TF functionality in separate module

f87a011

Avoid hard-coding TF resource renames for Chalice

dcdb286

Fix: TF schema check fails on Gitlab

60ed45f

… because no providers are installed.

hannes-ucsc force-pushed the issues/natanlao/1552-tag-azul-resources branch 2 times, most recently from cff5cf5 to 60ed45f Compare December 11, 2020 06:34

hannes-ucsc merged commit c0c29ad into develop Dec 11, 2020

hannes-ucsc deleted the issues/natanlao/1552-tag-azul-resources branch December 11, 2020 18:30

hannes-ucsc assigned melainalegaspi and unassigned hannes-ucsc Dec 11, 2020

natanlao mentioned this pull request Dec 18, 2020

Resources are untagged ucsc-cgp/cgp-cloud-policies#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tag AWS resources created with Terraform (#1552) #1701

Tag AWS resources created with Terraform (#1552) #1701

natanlao commented Apr 13, 2020 •

edited by hannes-ucsc

Loading

codecov bot commented Apr 13, 2020 •

edited

Loading

coveralls commented Apr 13, 2020 •

edited

Loading

jessebrennan left a comment

natanlao commented May 4, 2020

jessebrennan left a comment

natanlao commented May 13, 2020 •

edited

Loading

natanlao commented May 13, 2020 •

edited

Loading

jessebrennan left a comment

hannes-ucsc Nov 16, 2020

hannes-ucsc Nov 16, 2020

hannes-ucsc Nov 16, 2020

hannes-ucsc commented Dec 11, 2020

Tag AWS resources created with Terraform (#1552) #1701

Tag AWS resources created with Terraform (#1552) #1701

Conversation

natanlao commented Apr 13, 2020 • edited by hannes-ucsc Loading

codecov bot commented Apr 13, 2020 • edited Loading

Codecov Report

coveralls commented Apr 13, 2020 • edited Loading

Pull Request Test Coverage Report for Build 8318

💛 - Coveralls

jessebrennan left a comment

Choose a reason for hiding this comment

natanlao commented May 4, 2020

jessebrennan left a comment

Choose a reason for hiding this comment

natanlao commented May 13, 2020 • edited Loading

natanlao commented May 13, 2020 • edited Loading

jessebrennan left a comment

Choose a reason for hiding this comment

hannes-ucsc Nov 16, 2020

Choose a reason for hiding this comment

hannes-ucsc Nov 16, 2020

Choose a reason for hiding this comment

hannes-ucsc Nov 16, 2020

Choose a reason for hiding this comment

hannes-ucsc commented Dec 11, 2020

natanlao commented Apr 13, 2020 •

edited by hannes-ucsc

Loading

codecov bot commented Apr 13, 2020 •

edited

Loading

coveralls commented Apr 13, 2020 •

edited

Loading

natanlao commented May 13, 2020 •

edited

Loading

natanlao commented May 13, 2020 •

edited

Loading