design: samconfig by sriram-mv · Pull Request #1503 · aws/aws-sam-cli

sriram-mv · 2019-11-05T06:57:07Z

a configuration that provides parameter pass throughs for
sam cli commands
Write Design Document (Do I need to write a design document?)
Write unit tests
Write/update functional tests
Write/update integration tests
make pr passes
Write documentation

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

- an app level configuration that provides parameter pass throughs for sam cli commands

awood45

Some feedback.

awood45 · 2019-11-05T21:39:01Z

designs/sam-app-config.md

+
+* `sam package --s3-bucket ... --template-file ... --output-template-file ... --s3-prefix ... --kms-key-id ...`
+
+* `sam deploy deploy --template-file ... --stack-name ... --capabilities ... --tags ... --parameter-overrides ... --kms-key-id ...`


sam deploy deploy?

Good catch.

awood45 · 2019-11-05T21:40:51Z

designs/sam-app-config.md

+* `sam package`
+* `sam deploy`
+
+That would be a huge user experience win.


I'd add to the problem some more considerations:

Dev/Beta/PreProd/Prod workflows may vary and need to be reflected somehow.

There is a tie-in with ~/.aws/credentials and ~/.aws/config to respect, or at least to keep consistent in some way.

A solution should allow for overriding of any defaults - explicit parameter choices should always be respected.

Not necessarily. ~/.aws/credentials and ~/.aws/config are global files. Yes, explicit parameter will always be respected. I can add that under a section called tenets.

Those are global config files, but do we want the same profile identifier to work for both? Why or why not? Let's make a conscious decision.

We can always leave the door open for identifiers, but I'd prefer not to start with one. It may conflate expectations with aws profiles.

+1 to start with simple, as long as we keep door open to add profiles in future if needed

awood45 · 2019-11-05T21:45:49Z

designs/sam-app-config.md

+
+The suite of commands supported by SAM CLI would be aided by looking for a configuration file thats locally located to the template.yaml by default. 
+
+`.aws-sam/sam-app-config`


Is this an .aws-sam directory relative to the project? Would we want to conflate this with build directories like that?

yes, its where build artifacts go today.

I'd want to separate these concepts. I don't think there's any harm to having this expected to exist at project root, or to be otherwise specified. (On that note, we should support an ENV variable for this possibly).

awood45 · 2019-11-05T21:46:02Z

designs/sam-app-config.md

+
+Sample configuration file
+
+```


INI format?

Also, do we want to consider user profiles in a way that aligns with AWS CLI?

this is toml. looks similair to ini but understands types better. pip today respects pyproject.toml. I'd rather have multiple configuration files, similar to how parameter overrides usually denote different environments.

We should make sure toml supports what we want. I am ok with it but I prefer us to not support multiple configuration files. The upkeep on that might be more than we originally think.

awood45 · 2019-11-05T21:47:19Z

designs/sam-app-config.md

+Out-of-Scope
+------------
+
+* Not focusing on a global configuration. SAM CLI already has a notion of a global config at `~/.aws-sam/metadata.json`


Different in nature. In the future, could always support a global configuration file in ~/.aws-sam/ - it doesn't conflict with metadata.json existing. It's an intentional choice because a global config may not make sense for this problem.

awood45 · 2019-11-05T21:47:51Z

designs/sam-app-config.md

+region="us-east-1"
+profile="srirammv"
+```
+


It may be worth mentioning how this config file will be bootstrapped. By hand? By a helper command? Via init? Some combination?

This design does not cover for that scenario, but can account for potential directions forward.

jfuss · 2019-11-07T19:22:38Z

designs/sam-app-config.md

+
+The suite of commands supported by SAM CLI would be aided by looking for a configuration file thats locally located to the template.yaml by default. 
+
+`.aws-sam/sam-app-config`


Why sam-app-config? Should we simplify this further to samconfig?

I am confused on the placement within .aws-sam. To this point, this directory is generated by the cli directly (to ensure correct permissions, mainly). With this, we are suggestion to customers that they need to now create this directory. This might be ok if the entry point was through a command (sam config generate) but we aren't that far yet. Why not place this within the root of the project?

We can move it up to the project root, to me this configuration was very specific to the app the user was working on at that point. Also might prevent accidental checking it in to source control, since we ignore the .aws-sam directory.

sam-app-config because it doesn't change the inherent behavior of SAM CLI, its a pass through of parameters.

jfuss · 2019-11-07T19:25:50Z

designs/sam-app-config.md

+
+Sample configuration file
+
+```


We should make sure toml supports what we want. I am ok with it but I prefer us to not support multiple configuration files. The upkeep on that might be more than we originally think.

jfuss · 2019-11-07T19:27:19Z

designs/sam-app-config.md

+CLI Changes
+-----------
+
+New command line argument is added per command called `--config` to be able to specify non default locations of config files


Is there a reason we wouldn't want a command to not have the --config option?

generate-event consists of tons of subcommands, that may be a place where config might not be used as much. But we can add it across all commands.

jfuss · 2019-11-07T19:30:21Z

designs/sam-app-config.md

+What will be changed?
+---------------------
+
+The suite of commands supported by SAM CLI would be aided by looking for a configuration file thats locally located to the template.yaml by default. 


How does this work with build then? We have (at least try to) trained customers to operate on their source. In cases of build, we generate a new template for them (that they shouldn't care about). This means in some cases, we would pick the config in the source but others (that have run build before) we will be looking at the built template. Lets be strong on where this should live, otherwise this can be a point of confusion for customers.

Moved to project level with an option to set the location of the configuration file with a environment variable.

jfuss · 2019-11-07T19:31:03Z

designs/sam-app-config.md

+Documentation Changes
+=====================
+
+* Addition of a new `--config` file parameter per command


What about docs page about the config, what it does, how/why you should use it?

sriram-mv · 2019-11-11T18:07:05Z

open to modification of --identifier. it could be environment, mode, config.

sanathkr

This looks awesome! Very powerful capability. I know we are rushing this quick, but I would like if you can explain all the other ways this feature can be extended in the future such as global + project-level config, checking the config into Git repo, multiple config files with a naming convention, etc.

Also, can you elaborate on how a users can use --debug to understand how SAM CLI read the config?

sanathkr · 2019-11-11T20:26:16Z

designs/sam-config.md

+```
+
+
+The default location of a .sam-config can be replaced by overriding an environment variable called `SAM_CONFIG`


Is this consistent with other env vars names? Should we make this SAM_CLI_CONFIG?

sanathkr · 2019-11-11T20:26:23Z

designs/sam-config.md

+
+The suite of commands supported by SAM CLI would be aided by looking for a configuration file thats locally located at the project root where template.yaml is located by default. 
+
+`.sam-app-config`


sanathkr · 2019-11-11T20:51:04Z

designs/sam-config.md

+====================================
+
+
+What is the problem?


Provide a bigger scope

sanathkr · 2019-11-11T20:51:16Z

designs/sam-config.md

@@ -0,0 +1,282 @@
+SAM CLI App Level Config


Can we call this "SAM Config" in general then?

sanathkr · 2019-11-11T20:51:52Z

designs/sam-config.md

+Be default the `default` section of the configuration is chosen.
+
+```
+[default]


sanathkr · 2019-11-11T20:55:39Z

designs/sam-config.md

+region="us-east-1"
+profile="srirammv"
+
+[dev]


This makes much more sense to me. I like this

jfuss · 2019-11-11T22:27:17Z

designs/sam-config.md

+
+Users can pass an identifier for the section that will be scanned within the configuration file to pass parameters through.
+
+Be default the `default` section of the configuration is chosen.


nit: By default

jfuss · 2019-11-11T22:29:01Z

designs/sam-config.md

+skip_pull_image=true
+use_container=true
+
+[default.package]


Not overly familiar with TOML but is this required? What would we place in this section?

Its to showcase nesting, its theoretically not required, so I'll remove it for brevity.

jfuss · 2019-11-11T22:31:34Z

designs/sam-config.md

+
+* Potentially every sam command could have functionality to have a series of command line parameters exported into a configuraion file.
+
+Tenets


Nit: consider moving this up before "What will be changed?". Might help set some context and be easier to reference and update when we make changes in the future.

- todo add more scope

awood45 · 2019-11-12T23:04:51Z

designs/sam-config.md

+
+Optionally, if multiple configuration files are checked in. One can change the `SAM_CLI_CONFIG` environment variable to point a different configuration file.
+
+`--env` can also be passed in to deal with custom environments defined in the configuration file.


Could we use a different parameter name than this, something more descriptive/connected to the config file?

I'm open to suggestions.

sanathkr · 2019-11-12T23:06:40Z

designs/sam-config.md

+* Read `--env` to make sure we can select an appropriate portion in configuration file.
+
+
+`.samrc` Changes


This PR is samrc. Can you actually rename this part of the design doc template to call it samconfig instead?

sanathkr · 2019-11-12T23:07:17Z

designs/sam-config.md

+
+`
+sam build --env devo
+Error: Environment 'devo' was not found in .aws-sam/samconfig.toml , Possible environments are : ['dev', 'prod']


Good job on the thinking ahead about error messages.

sanathkr · 2019-11-12T23:07:35Z

designs/sam-config.md

+Test Scenarios/Cases
+--------------------
+
+* Integration tests for every command with identifer based overrides, and command line overrides on existing sam configuration file and custom configuration file through environment variables.


s/identifier/env

sanathkr · 2019-11-12T23:08:33Z

designs/sam-config.md

+
+N/A
+
+Test Scenarios/Cases


How do you plan to verify that the config file with every single CLI command we have today and in future?

Lets add a configuration file version number

sanathkr · 2019-11-12T23:12:08Z

designs/sam-config.md

+Documentation Changes
+=====================
+
+* Addition of a new `--env` parameter per command


Also need documentation on how to write the config file. How to specify flags like --debug that don't have explicit values. How to verify whether a config file is parsable or not.

* design: sam-app-config - an app level configuration that provides parameter pass throughs for sam cli commands * design: move config file location to project root * design: add open questions * fix: add `identifiers` to use configuration file. * fix: address comments - todo add more scope * fix: address future and scope * phases: implementation phases * fix: add docs section for samconfig

design: sam-app-config

6f8238b

- an app level configuration that provides parameter pass throughs for sam cli commands

sriram-mv mentioned this pull request Nov 5, 2019

feat: configuration file for sam cli commands #1502

Merged

6 tasks

awood45 reviewed Nov 5, 2019

View reviewed changes

jfuss suggested changes Nov 7, 2019

View reviewed changes

sriram-mv added 3 commits November 7, 2019 16:04

design: move config file location to project root

cff9bc0

design: add open questions

8a974dd

fix: add identifiers to use configuration file.

31ec4dd

sanathkr mentioned this pull request Nov 11, 2019

sam build --parameter-overrides fails to override #1490

Closed

sanathkr suggested changes Nov 11, 2019

View reviewed changes

jfuss reviewed Nov 11, 2019

View reviewed changes

fix: address comments

fc91010

- todo add more scope

sriram-mv force-pushed the sam-app-config-design branch from 2273fc8 to fc91010 Compare November 12, 2019 06:46

jfuss added the type/design label Nov 12, 2019

sriram-mv changed the title ~~design: sam-app-config~~ design: samconfig Nov 12, 2019

sriram-mv added 2 commits November 12, 2019 14:56

fix: address future and scope

e715908

phases: implementation phases

0c4b0c6

awood45 reviewed Nov 12, 2019

View reviewed changes

sanathkr approved these changes Nov 12, 2019

View reviewed changes

fix: add docs section for samconfig

a5835e6

sriram-mv changed the base branch from develop to release-v0.32.0 November 13, 2019 19:00

sriram-mv merged commit 865e6b3 into aws:release-v0.32.0 Nov 13, 2019


		* `sam package --s3-bucket ... --template-file ... --output-template-file ... --s3-prefix ... --kms-key-id ...`

		* `sam deploy deploy --template-file ... --stack-name ... --capabilities ... --tags ... --parameter-overrides ... --kms-key-id ...`


		The suite of commands supported by SAM CLI would be aided by looking for a configuration file thats locally located to the template.yaml by default.

		`.aws-sam/sam-app-config`

		```


		The default location of a .sam-config can be replaced by overriding an environment variable called `SAM_CONFIG`


		The suite of commands supported by SAM CLI would be aided by looking for a configuration file thats locally located at the project root where template.yaml is located by default.

		`.sam-app-config`


		Users can pass an identifier for the section that will be scanned within the configuration file to pass parameters through.

		Be default the `default` section of the configuration is chosen.


		* Potentially every sam command could have functionality to have a series of command line parameters exported into a configuraion file.

		Tenets


		Optionally, if multiple configuration files are checked in. One can change the `SAM_CLI_CONFIG` environment variable to point a different configuration file.

		`--env` can also be passed in to deal with custom environments defined in the configuration file.

		* Read `--env` to make sure we can select an appropriate portion in configuration file.


		`.samrc` Changes


		N/A

		Test Scenarios/Cases

Conversation

sriram-mv commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awood45 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sriram-mv commented Nov 11, 2019

Uh oh!

sanathkr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sriram-mv commented Nov 5, 2019 •

edited

Loading