fix(cdk): merge cloudFormation tags with aspect tags #1762

moofish32 · 2019-02-14T06:34:07Z

This modifies the behavior of TagManager to enable the merging of tags
provided during Cfn* properties with tag aspects. If a collision occurs
the aspects tag precedence.

fixes #1725

Pull Request Checklist

Testing
- Unit test added
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

This modifies the behavior of TagManager to enable the merging of tags provided during Cfn* properties with tag aspects. If a collision occurs the aspects tag precedence. fixes aws#1725

moofish32

I've enabled the merging of tags with L1s but aspects to precedence. If we have an end goal that L2 constructs must be configurable by pure properties and no methods. This makes me think I should refactor all the places we have used aspects in L2's to actually go back and set L1 props. I don't have a pure quantitive reason for that, just feels more correct. What does the team think here?

moofish32 · 2019-02-14T06:35:16Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

-  AutoScalingGroup = 'AutoScalingGroupTag',
-  Map = 'StringToStringMap',
-  NotTaggable = 'NotTaggable',
+    AutoScalingGroup = 'AutoScalingGroupTag',


I'll fix this -- not sure where I need to fix this in my auto-formatter 👎

moofish32 · 2019-02-14T06:36:30Z

packages/@aws-cdk/cdk/lib/core/tag-manager.ts

@@ -150,4 +151,45 @@ export class TagManager {
    }
    return true;
  }
+
+  private mergeFrom(propertyTags: any): void {


I think I might be ready to refactor to some form a subclass or composition class for each type of tag. These switch statements are not clean in my opinion. Do you think it's worth it at this point?

I totally think that's a good idea. Should there not be instances of TagFormatter that live on a taggable Resource?

moofish32 · 2019-02-14T06:44:53Z

I unchecked tests I think I owe at least two more for the testing the raise statements.

rix0rrr · 2019-02-14T12:46:45Z

packages/@aws-cdk/cdk/test/aspects/test.tag-aspect.ts

@@ -130,7 +130,7 @@ export = {
    test.deepEqual(res2.tags.renderTags(), [{key: 'key', value: 'value'}]);
    test.done();
  },
-  'Aspects are mutually exclusive with tags created by L1 Constructor'(test: Test) {
+  'Aspects are merged with tags created by L1 Constructor'(test: Test) {


rix0rrr · 2019-02-14T12:51:18Z

packages/@aws-cdk/cdk/lib/core/tag-manager.ts

@@ -94,7 +94,8 @@ export class TagManager {
  /**
   * Renders tags into the proper format based on TagType
   */
-  public renderTags(): any {
+  public renderTags(propertyTags?: any): any {
+    this.mergeFrom(propertyTags);


I find this slightly scary because it will change the state of the TagManager class. If renderTags() gets called twice, do we know it does the same the 2nd time around?

I'd prefer merging on a local variable.

agree - because of the current design it was accidentally idempotent, not the right thing.

moofish32 · 2019-02-16T22:45:29Z

@eladb @rix0rrr -- I'm now seeing:

@aws-cdk/cdk: [2019-02-16T14:43:04.217] [ERROR] jsii/compiler - Type model errors prevented the JSII assembly from being created
@aws-cdk/cdk: lib/core/tag-manager.ts:50:33 - error JSII0: Only string index maps are supported
@aws-cdk/cdk: 50   renderTags(propertyTags: any, tags: Tags ): {tags: any};
@aws-cdk/cdk:                                    ~~~~~~~~~~

this is no longer allowed is my guess?

/**
 * Properties Tags is a dictionary of tags as strings
 */
type Tags = { [key: string]: {value: string, props: TagProps }};

/**
/**
 * Properties for a tag
 */
export interface TagProps {
  /**
   * Handles AutoScalingGroup PropagateAtLaunch property
   */
  applyToLaunchedInstances?: boolean;

  /**
   * An array of Resource Types that will not receive this tag
   *
   * An empty array will allow this tag to be applied to all resources. A
   * non-empty array will apply this tag only if the Resource type is not in
   * this array.
   * @default []
   */
  excludeResourceTypes?: string[];

  /**
   * An array of Resource Types that will receive this tag
   *
   * An empty array will match any Resource. A non-empty array will apply this
   * tag only to Resource types that are included in this array.
   * @default []
   */
  includeResourceTypes?: string[];

  /**
   * Higher or equal priority tags will take precedence
   *
   * Setting priority will enable the user to control tags when they need to not
   * follow the default precedence pattern of last applied and closest to the
   * construct in the tree.
   * @default 0 for Tag 1 for RemoveTag
   */
  priority?: number;
}

Specifically the use of type Tags up there?

…ypes; adding in additional tests

fully defined

…lt tag type

moofish32 · 2019-02-18T05:50:32Z

@eladb @rix0rrr - I refactored to eliminate what I think might be the JSII issue too which is the complex type. This means I needed to use a simple dictionary to manage an array of objects.

A question on the precedence in this PR. The current position is aspects will override L1 prop tags. This seems weird given our precedence of closest to the node is the chosen value. The L1 property seems to be the closest, but we are not letting it overwrite the aspect. I've coded this so we can easily reverse this thinking. What do you all think?

rix0rrr · 2019-02-18T08:37:49Z

A question on the precedence in this PR. The current position is aspects will override L1 prop tags.

Given that we have an L2 mechanism, who would ever set L1 tags? L2 writers shouldn't because they can use the L2 mechanism. Is there a use case in which this is logical? Or is this because of legacy code that right now uses an L1 mechanism which we need to rewrite to use the L2 mechanism?

moofish32 · 2019-02-18T16:41:53Z

@rix0rrr - The description here is how this started. Technically, it's not as simple as order in these cases. As long as users can access L1 tags though, then this issue occurs.

rix0rrr · 2019-02-20T13:38:55Z

packages/@aws-cdk/cdk/lib/core/tag-manager.ts

@@ -44,16 +40,31 @@ export interface TagProps {
  priority?: number;
 }

+export interface ITag {


According to our naming standards the name of this interface should not start with an I since it doesn't have any behavior.

rix0rrr · 2019-02-20T13:47:21Z

packages/@aws-cdk/cdk/lib/core/tag-manager.ts

+    return formatter.renderTags(tags, propertyTags);
+  }
+
+  private tagFormatter(): ITagFormatter {
    switch (this.tagType) {


tagType is only used to pick the right type of formatter. So we could replace the whole enum meber with an instance of ITagFormatter. We could have this instead:

class TagManager { constructor(private readonly tagFormatter: ITagFormatter) { } public renderTags(...) { this.formatter.renderTags(...); } }

Muy simpler!

If you want to keep the enum for the caller's convenience, do the lookup in the constructor. You can also use an object to house the lookup map:

class TagManager { private readonly tagFormatter: ITagFormatter; constructor(tagType: TagType) { this.tagFormatter = TAG_FORMATTERS[tagType]; } } const TAG_FORMATTERS = { [TagType.ASG]: new AsgFormatter(), [TagType.Normal]: new NormalFormatter(), ... }

I preferred to leave the codegen with just the simple enum and moved to the const TAG_FORMATTERS ... pattern. If you strongly otherwise I'll move it to codegen.

rix0rrr · 2019-02-20T13:49:29Z

packages/@aws-cdk/cdk/lib/core/tag-manager.ts

-  private readonly tags: Tags = {};
-
+  private readonly tags: ITag[] = [];
+  private readonly tagSet: {[key: string]: number} = {};


I'm probably thick, but I'm going to need more information on what this member does. It seems to be mapping a tag name to an index in the the tags array, but why? Is it just for fast lookup by name?

Why do we not have something like this instead:

class TagManager { private readonly tags = new Map<string, ITag>(); /** * Return all active tags */ public get currentlyActiveTags: ITag[] { return Array.from(this.tags.values()); } }

If the goal is to be able to switch tags off on demand (by using RemoveTag) while not completely losing track of them, another data structure in the Map<> to keep that state will do nicely:

interface AppliedTag { tagObject: ITag; active: boolean; // true if last 'set', false if last 'remove'd. appliedAtPriority: number; // Priority of last change } class TagManager { private readonly tags = new Map<string, AppliedTag>(); /** * Return all active tags */ public get currentlyActiveTags: ITag[] { return Array.from(this.tags.values()).filter(t => t.active).map(t => t.tagObject); } }

This whole confusion was me not understanding the JSII error above was telling me that I was leaking a private type. Refactored this to use Map where appropriate.

rix0rrr · 2019-02-20T14:02:25Z

packages/@aws-cdk/cdk/lib/core/tag-manager.ts

+  }
+}
+
+function mergeTags(target: any, source: any): any {


I don't particularly like that this thing can handle both arrays and objects, because we lose type information this way. It should just handle arrays, and the MapFormatter should just do the object assignment directly.

Fair - I did this so I could later flip the precedence, but moved it back as you requested.

moofish32 · 2019-02-21T08:27:21Z

@rix0rrr -- need a little help here: pack.sh returns successfully

...
dist//sphinx/__aws-cdk_aws-servicecatalog.README.md
dist//sphinx/_aws-cdk_aws-opsworks.rst
dist//sphinx/__aws-cdk_aws-directoryservice.README.md
dist//sphinx/_aws-cdk_aws-s3-deployment.rst
dist//sphinx/_aws-cdk_aws-neptune.rst
dist//sphinx/_aws-cdk_aws-kinesisanalytics.rst
dist//sphinx/__aws-cdk_aws-codepipeline.README.md
dist//sphinx/__aws-cdk_aws-iot1click.README.md
dist//sphinx/_aws-cdk_aws-autoscaling-common.rst
dist//build.json

What else is in the codebuild job that's not in Travis?

moofish32 · 2019-02-21T09:38:16Z

Hmm - looks like a testing blip. Let me know if you see something different.

rix0rrr · 2019-02-21T13:50:43Z

packages/@aws-cdk/cdk/lib/core/tag-manager.ts

-    if (this.tags[key]) {
-      if (this.tags[key].props.priority !== undefined) {
-        return priority >= this.tags[key].props.priority!;
+    if (this.tags.has(key)) {


I think this logic can be simplified a lot. First of all, I don't really like that undefined priorities are retained this way in the state undefined and that it factors into the comparison in some way.

I would assume all tag applications always have a priority, but the user can avoid specifying it and then it gets a default value. I would also pick default values higher than 0 and 1, maybe with more possible values in between them. Let's say 100 and 200. By keeping slightly different state, I think we can simplify to this:

class TagManager { private readonly tags = new Map<string, { ... }>(); // Values of currently active tags private readonly priorities = new Map<string, number>(); // Priorities of ALL applied changes private canApplyTag(key: string, priority: number): boolean { const prio = this.priorities.get(key) || 0; return priority >= number; } public setTag(key: string, value: string, props: TagProps = {}) { const prio = props.priority !== undefined ? props.priority : 100; if (!this.canApplyTag(key, prio)) { return; } this.tags.set(key, { value, props }); this.priorities.set(key, prio); } public removeTag(key: string, props: TagProps = {}) { const prio = props.priority !== undefined ? props.priority : 200; if (!this.canApplyTag(key, prio)) { return; } this.tags.delete(key); this.priorities.set(key, prio); // Priority is always set } }

Also, ultimately I also think it's more understandable (and flexible) if we assign the L1 tags also a (virtual) priority. Let's say 50, so that it's lower than the L2 tags BY DEFAULT (but can be changed).

Now, I understand that doesn't fit in the current model really well, where we don't even know the TYPE of the L1 tags until we get to the renderer, so I won't make you change it. But in my head, there should be a sort of algebra of tags, with which we could implement renderTags() simply like so:

public renderTags(propertyTags: SomeType): any { const l1Tags = TagSet.fromL1Tags(propertyTags, 50); const l2Tags = this.tagSet; const finalTags = l1Tags.merge(l2Tags); // merge() knows about priorities and tombstones return this.tagFormatter(finalTags); }

@rix0rrr - good feedback.

Quick follow is this a push to remove include/exclude by resource type?

I think the crux of this issue is:

Now, I understand that doesn't fit in the current model really well, where we don't even know the TYPE of the L1 tags until we get to the renderer, so I won't make you change it.

What if we change that? We have a format for tag input that is agnostic to output. If I change the code generation to generate a tag type that is uniform (for input) and unique on output (to keep validation the same as it is today), then this issue disappears. If we look at the current tag input structures:

tags?: object | cdk.Token; // OR tags?: Array<cdk.CfnTag | cdk.Token> | cdk.Token; // OR tags?: Array<CfnAutoScalingGroup.TagPropertyProperty | cdk.Token> | cdk.Token; // We standardize to interface CdkTag { // not sure on this name yet key: string; value: string; // default 50 or whatever we decide from above priority?: number; // default true applyToLaunchedInstances?: boolean; } tags?: CdkTag[];

I think with our ability to treat tokens as strings and the ability to now add tags later via aspect we no longer need the ... | cdk.Token definitions anywhere. Now @eladb might beat me up on ergonomics here. Yes, I agree simple key, value would be better. However, this keeps the L1 in a true declarative state. The rule I'm clearly breaking here is, this is the first input property that doesn't directly match CloudFormation. If somebody was writing a generator to turn templates into CDK files this would now require special code. I personally think the standardization is a bigger win, and later if we do a declarative CDK at the L2 level we have a consistent definition of tag.

Then in the constructor to TagManager we can pass in initial tags and they are effectively applied immediately because these tags do not propagate like an aspect. Now we roll in your changes from above and I think this is a much cleaner code solution where merge isn't really necessary. If it is necessary we can create the merge on a tag class that does this algebra as you describe.

Counter-proposal: we keep the L1 type definitions as they are (I do think there's value in sticking closely to the CFN model, if only for predictability and explainability), AND we define the one uber-type of tag, and we make the TagFormatter be able to convert between them:

interface ITagFormatter { parseTags(l1tags: any): CdkUberTag[]; formatTags(tags: CdkUberTag[]): any; }

So in fact, the tag formatter becomes a tag converter, back and forth from our unified L2 type to the various L1 types.

I would type the any more strictly there, but doing so would require generics and I think we might run afoul of JSII. It's going to require unnatural gymnastics to keep the definition of ITagFormatter out of reach of the JSII parser; it's probably simpler to type as any and add some runtime checks.

And then we can still use TagFormatter/Converter in the constructor to get the initial set of tags applied and we don't need to do any merging.

The only complication would be Tokens if they were used to return a whole set of tags at once. I'm perfectly fine with detecting that case (using cdk.unresolved()) and throwing an error, saying we don't allow it.

packages/@aws-cdk/cdk/README.md

…ogic, set priority defaults with space between values, code generation now passes in L1 tags

moofish32

@rix0rrr I think I have captured your feedback in these changes, added a few comments to highlight my thoughts.

packages/@aws-cdk/cdk/lib/aspects/tag-aspect.ts

moofish32 · 2019-03-05T15:12:28Z

packages/@aws-cdk/cdk/lib/cloudformation/tag.ts

 /**
 * @link http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-resource-tags.html
 */
 export interface CfnTag {
  /**
   * @link http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-resource-tags.html#cfn-resource-tags-key
   */
-  key: string;
+  key: string | Token;


I did this here because we talked about it in the PR. However, the code generation for ASG does not add tokens here any longer. Do we still want this?

I would say we don't.

packages/@aws-cdk/cdk/lib/core/tag-manager.ts

moofish32 · 2019-03-06T18:56:49Z

packages/@aws-cdk/cdk/lib/core/tag-manager.ts

-      return priority >= this.removedTags[key];
-    }
-    return true;
+    return priority >= (this.priorities.get(key) || 0);


Is this simplification enough? or did you want me to look at removing the include/exclude checks above as well.

moofish32 · 2019-03-06T18:57:51Z

tools/cfn2ts/lib/codegen.ts

@@ -302,6 +302,10 @@ export default class CodeGenerator {
    if (deprecated) {
      this.code.line(`this.node.addWarning('DEPRECATION: ${deprecation}');`);
    }
+    if (tagEnum !== `${TAG_TYPE}.NotTaggable`) {
+      this.code.line('const tags = props === undefined ? undefined : props.tags;');
+      this.code.line(`this.tags = new ${TAG_MANAGER}(${tagEnum}, ${resourceTypeName}, tags);`);


Added this to eliminate the "merge" concept.

moofish32 · 2019-03-06T18:58:46Z

tools/cfn2ts/lib/codegen.ts

@@ -581,7 +588,7 @@ export default class CodeGenerator {
        if (union.indexOf('|') !== -1) {
          alternatives.push(`Array<${union}>`);
        } else {
-          alternatives.push(`(${union})[]`);
+          alternatives.push(`${union}[]`);


was this dead code? It broke for me and changing this fixes it, but it does leave me asking how did this work if it wasn't dead code?

rix0rrr

Thanks! This looks great!

There are a couple of small things that I'd like to see changed, but I will take care of them myself. Biggest thing I see is that TagProps is fulfilling two roles: it's both aspect input as well as used in processing.

I think those should be separate datastructures for different purposes. I'll show you later on.

rix0rrr · 2019-03-13T13:43:33Z

packages/@aws-cdk/cdk/lib/cloudformation/tag.ts

 /**
 * @link http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-resource-tags.html
 */
 export interface CfnTag {
  /**
   * @link http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-resource-tags.html#cfn-resource-tags-key
   */
-  key: string;
+  key: string | Token;


I would say we don't.

packages/@aws-cdk/cdk/lib/core/tag-manager.ts

… defined The defaulting happens at the surface that the user interacts with, which makes the backend easier to write because it has less case analysis and exceptions to think about.

rix0rrr · 2019-03-13T14:52:09Z

Okay I'm going to have to eat crow :). I see the constraints you were working under now. Sorry!

moofish32 · 2019-03-13T15:11:08Z

Thanks @rix0rrr @eladb - I'll head over to look at L2 tags next.

fix(cdk): merge cloudFormation tags with aspect tags

9a3fade

This modifies the behavior of TagManager to enable the merging of tags provided during Cfn* properties with tag aspects. If a collision occurs the aspects tag precedence. fixes aws#1725

moofish32 requested a review from a team as a code owner February 14, 2019 06:34

moofish32 commented Feb 14, 2019

View reviewed changes

rix0rrr reviewed Feb 14, 2019

View reviewed changes

moofish32 added 3 commits February 17, 2019 16:19

refactoring tag-manager to avoid JSII errors for complex dictionary t…

65c0596

…ypes; adding in additional tests

removing export from classes that do not need it, asg tag interface

18ce625

fully defined

can not resolve early; note - go back and consider changing the defau…

4305741

…lt tag type

rix0rrr reviewed Feb 20, 2019

View reviewed changes

refactor tags using Map

708eff3

trigger retest'

455d697

rix0rrr reviewed Feb 21, 2019

View reviewed changes

eladb mentioned this pull request Mar 4, 2019

Tags property in L2s #1935

Closed

eladb reviewed Mar 4, 2019

View reviewed changes

packages/@aws-cdk/cdk/README.md Show resolved Hide resolved

moofish32 added 3 commits March 5, 2019 00:20

refactoring to enable tag formatters to parse L1 tags, remove merge l…

15aa430

…ogic, set priority defaults with space between values, code generation now passes in L1 tags

Merge branch 'master' into b-child-tag-merge

15da077

fixing codegen.ts missing arg to constructor

8eebcc9

moofish32 commented Mar 6, 2019

View reviewed changes

rix0rrr added 3 commits March 13, 2019 14:43

Remove Tokens from cfntag.

87eca86

'removedTags' is unused now

9d8119b

Default argument to empty object

d0281ee

rix0rrr reviewed Mar 13, 2019

View reviewed changes

Refactor a bit so that the TagManager works with tags that are mostly…

e026e52

… defined The defaulting happens at the surface that the user interacts with, which makes the backend easier to write because it has less case analysis and exceptions to think about.

Remove the internal 'Tag' type from public exposure

5724be9

rix0rrr approved these changes Mar 13, 2019

View reviewed changes

rix0rrr merged commit bfb14b6 into aws:master Mar 13, 2019

This was referenced Dec 12, 2019

[Snyk] Upgrade @aws-cdk/cdk from 0.22.0 to 0.35.0 MechanicalRock/scdk#3

Closed

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/cdk-constructs#8

Open

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/account-reaper#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cdk): merge cloudFormation tags with aspect tags #1762

fix(cdk): merge cloudFormation tags with aspect tags #1762

moofish32 commented Feb 14, 2019 •

edited

Loading

moofish32 left a comment •

edited

Loading

moofish32 Feb 14, 2019

moofish32 Feb 14, 2019

rix0rrr Feb 14, 2019

moofish32 commented Feb 14, 2019

rix0rrr Feb 14, 2019

rix0rrr Feb 14, 2019

moofish32 Feb 14, 2019

moofish32 commented Feb 16, 2019

moofish32 commented Feb 18, 2019

rix0rrr commented Feb 18, 2019

moofish32 commented Feb 18, 2019

rix0rrr Feb 20, 2019

rix0rrr Feb 20, 2019

moofish32 Feb 21, 2019

rix0rrr Feb 20, 2019

moofish32 Feb 21, 2019

rix0rrr Feb 20, 2019

moofish32 Feb 21, 2019

moofish32 commented Feb 21, 2019

moofish32 commented Feb 21, 2019

rix0rrr Feb 21, 2019

moofish32 Feb 24, 2019

rix0rrr Feb 25, 2019 •

edited

Loading

moofish32 left a comment

moofish32 Mar 5, 2019

rix0rrr Mar 13, 2019

moofish32 Mar 6, 2019

moofish32 Mar 6, 2019

moofish32 Mar 6, 2019

rix0rrr left a comment

rix0rrr Mar 13, 2019

rix0rrr commented Mar 13, 2019

moofish32 commented Mar 13, 2019

fix(cdk): merge cloudFormation tags with aspect tags #1762

fix(cdk): merge cloudFormation tags with aspect tags #1762

Conversation

moofish32 commented Feb 14, 2019 • edited Loading

Pull Request Checklist

moofish32 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 commented Feb 16, 2019

moofish32 commented Feb 18, 2019

rix0rrr commented Feb 18, 2019

moofish32 commented Feb 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 commented Feb 21, 2019

moofish32 commented Feb 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

moofish32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Mar 13, 2019

moofish32 commented Mar 13, 2019

moofish32 commented Feb 14, 2019 •

edited

Loading

moofish32 left a comment •

edited

Loading

rix0rrr Feb 25, 2019 •

edited

Loading