Skip to content

Feature/dynamo state#49

Open
jasdbrown wants to merge 37 commits into
developmentfrom
feature/dynamo-state
Open

Feature/dynamo state#49
jasdbrown wants to merge 37 commits into
developmentfrom
feature/dynamo-state

Conversation

@jasdbrown

Copy link
Copy Markdown
Contributor

No description provided.

@jasdbrown jasdbrown requested review from Pharrox and dejonghe May 7, 2020 14:59
Comment thread deployer/configuration.py Outdated
Comment thread deployer/configuration.py Outdated
Comment thread deployer/configuration.py
Comment thread deployer/__init__.py
parser.add_argument("-T", "--timeout", type=int, help='Stack create timeout')
parser.add_argument("-O", "--export-yaml", help="Export stack config to specified YAML file.",default=None)
parser.add_argument("-o", "--export-json", help="Export stack config to specified JSON file.",default=None)
parser.add_argument("-i", "--config-version", help="Execute ( list | get | set ) of stack config.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel like config-version isn't super clear that it is referring to a config action.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I had previously agreed with you until I saw it used in the read me.
We could leave it as is, move these to -x, or make a new CLI utility.

I think moving to -x probably makes the most sense.

deployer -s some-stack-name -x config-list

@jasdbrown do you have any input on that? Do these flags cause a completely different operation? If I use -i with -x update what happens?

Comment thread deployer/__init__.py
if not args.all:
if not args.execute:
if args.config_version:
if args.config_version != "list" and args.config_version != "set" and args.config_version != "get":

@yeslayla yeslayla May 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel that if not args.config_version in ["list", "set", "get"]: might be more clear code. I'm guessing it's not as performant though.

Comment thread deployer/cloudformation.py

@yeslayla yeslayla left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like a great addition to me. I've used it to test out a handful of different stacks with lambdas, the vpc stack, nested stacks, and existing environments and had no issues.

@dejonghe dejonghe left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nothing currently blocking but would like some conversation on the items I raised.

Comment thread README.md
Comment on lines 283 to +284
**Note**
Network Class has been removed, it's irrelivant now. It was in place because of a work around in cloudformation limitations. The abstract class may not be relivant, all of the methods are simmular enough but starting this way provides flexablility if the need arise to model the class in a different way.
Network Class has been removed, it's irrelevant now. It was in place because of a work around in cloudformation limitations. The abstract class may not be relivant, all of the methods are simmular enough but starting this way provides flexablility if the need arise to model the class in a different way.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Lets remove this. Anyone that even recalls that is probably long gone.

Comment thread README.md

```
shared-deployer:
stack_name: shared-deployer

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe comment on this line like:

stack_name: shared-deployer # No longer required

Comment thread deployer/__init__.py
parser.add_argument("-T", "--timeout", type=int, help='Stack create timeout')
parser.add_argument("-O", "--export-yaml", help="Export stack config to specified YAML file.",default=None)
parser.add_argument("-o", "--export-json", help="Export stack config to specified JSON file.",default=None)
parser.add_argument("-i", "--config-version", help="Execute ( list | get | set ) of stack config.")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I had previously agreed with you until I saw it used in the read me.
We could leave it as is, move these to -x, or make a new CLI utility.

I think moving to -x probably makes the most sense.

deployer -s some-stack-name -x config-list

@jasdbrown do you have any input on that? Do these flags cause a completely different operation? If I use -i with -x update what happens?

Comment thread deployer/configuration.py

class Config(object):
def __init__(self, file_name, master_stack):
def __init__(self, profile, stack_name, file_name=None, override_params=None):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

profile can default to None

Comment thread deployer/tests.py
Comment on lines +372 to +373
#self.assertIn('update', [x['Value'] for x in tags if x['Key'] == 'Local'])
#self.assertIn('update', [x['Value'] for x in tags if x['Key'] == 'Override'])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why are these commented?

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