Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Use automatic resource naming for S3 buckets #132

Merged
merged 2 commits into from
Jul 23, 2015
Merged

Conversation

sevenmachines
Copy link
Contributor

Currently we use hard coded names for resources like S3. This change
will allow us to use dynamic names auto-generated by aws and exposed
as an output. (Closes #123)

  • S3 bucket names are optional in config, default to dynamic ones
  • Add a method and fab_file function to allow getting a filtered
    list of resources
  • Update tests to look for new BucketName output and policy changes
  • PEP8 fixes

@sevenmachines sevenmachines force-pushed the automatic_resource_naming branch 2 times, most recently from 395beaf to b9f027a Compare July 20, 2015 12:43
@sevenmachines sevenmachines force-pushed the automatic_resource_naming branch 5 times, most recently from be4ce4a to b10f42f Compare July 20, 2015 15:11
@sevenmachines sevenmachines changed the title (WIP) Use a automatic resource naming (WIP) Use automatic resource naming Jul 20, 2015
@sevenmachines sevenmachines force-pushed the automatic_resource_naming branch 4 times, most recently from fdcbbf5 to 98ef760 Compare July 21, 2015 08:08
@sevenmachines sevenmachines changed the title (WIP) Use automatic resource naming Use automatic resource naming Jul 21, 2015
# get the resources
resources = filter(fn, stack[0].list_resources())
else:
resources = stack[0].list_resources()
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be written a bit clearer. In pseudo code:

resources = stack.list_resources()
if resource_type:
  resources = apply filter(resources)

The rule (which is never a hard and fast rule, cos they never are) to apply is that if you can easily limit if/else then it's easier to follow the code path through.

@ashb
Copy link
Contributor

ashb commented Jul 21, 2015

Apart from the minor fixes mentioned in comments the change looks good, but this is only part of the changes needed for #123 - we also need to adjust ELBs and RDS instances similarly.

I'm happy to merge this change before then but we need to change the PR and commit message to not close the issue as it's not complete yet.

@sevenmachines
Copy link
Contributor Author

Fixup commit for review.
Note, I've seperated out the ELB and RDS equivalents into their own issues so closing this one is ok

@ashb ashb changed the title Use automatic resource naming Use automatic resource naming for S3 buckets Jul 22, 2015
Currently we use hard coded names for resources like S3. This change
will allow us to use dynamic names auto-generated by aws and exposed
as an output. (Closes #123)

- S3 bucket names are optional in config, default to dynamic ones
- Add a method and fab_file function to allow getting a filtered
list of resources
- Update tests to look for new BucketName output and policy changes
- Add test to check config with s3 key but no subkeys loads ok
- PEP8 fixes
@sevenmachines
Copy link
Contributor Author

Have squashed and rebased

"StaticBucketName",
Description="S3 bucket name",
Value=Ref("StaticBucket")
))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok time to refactor here - we shouldn't be creating things outside of the s3 function that relate to things inside it.

Options I can think of:

  • lists-of-lists from s3()

  • we could pass the Troposphere template object in to the class and let the s3 call add_resource or add_output as it needs.

  • We can return a list of objects and detect where to add them. Something like:

        objs = self.s3()
        self.add_to_template(template, objs)
    
    #  …
    
    def add_to_template(self, template, objects)
        for object in objs:
            if isinstance(obj, troposphere.Output):
                template.add_output(obj)
              elif isinstance(obj, troposphere.Parameter):
                template.add_parameter(obj)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used the 'returning the output list' option with ELB but I'm moving more towards the 'passing the template' option

@sevenmachines sevenmachines force-pushed the automatic_resource_naming branch 2 times, most recently from 1f488da to 63922fb Compare July 23, 2015 08:25
@sevenmachines
Copy link
Contributor Author

Fixups added,

  • pass template to s3
  • [arn] array change
  • Update to tests to handle template being passed to s3
  • static-bucket-name test change

@sevenmachines
Copy link
Contributor Author

Updated testing to cover s3 outputs values as well

ashb added a commit that referenced this pull request Jul 23, 2015
Use automatic resource naming for S3 buckets
@ashb ashb merged commit e8614fd into master Jul 23, 2015
@ashb ashb deleted the automatic_resource_naming branch July 23, 2015 14:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let Cloudformation generate names for S3 resources
2 participants