-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: Add sam publish command design doc #844
Conversation
d515ee9
to
7171465
Compare
3098f83
to
10eb646
Compare
Currently, when an app is first created by
The permission state is different between 1 and 3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks really good! Only blocker for me is the change in design around sharing (1 option instead of 3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to remove mention of permissions in a few places, but other than that, this looks solid!
333c0b7
to
a1a5f85
Compare
designs/sam_publish_app_cmd.rst
Outdated
... | ||
|
||
Build Lambda source code | ||
Run ``sam build -t template.yaml -b ./build -o built-template.yaml`` to build all functions in the template and output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the future: sam app publish
could trigger a build if there's none so one could have the following ideal workflow:
sam init
make changes to SAM and define a app locally
sam deploy
and test whether that workssam publish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is improvement outside of this command scope and something we are thinking about wider (invoke, package, etc) doing this build if not there.
designs/sam_publish_app_cmd.rst
Outdated
to upload code artifacts, readme and license files to S3 and generate the packaged template. | ||
|
||
Create new application in SAR | ||
Run ``sam publish app -t ./packaged.yaml`` to publish a new application named my-app in SAR with the first version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great to have packaged.yaml
as a default similarly to sam deploy
does it today as it's less to remember and more straightforward from a CLI UX :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should cover this when doing the migration of package.
Note: sam deploy
doesn't have a default value for the template Option.
designs/sam_publish_app_cmd.rst
Outdated
|
||
>>> sam publish app -t ./packaged.yaml | ||
Publish Succeeded | ||
Created new application with the following metadata: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of giving metadata of the published app (e.g. sam app info instead) it'd be great to leave a AWS::Serverless::Application
resource filled out with the information about this app newly published so one can go copy/paste into somewhere (slack, template, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! This can be added in a follow-up iteration to output CFN resource of the published app.
designs/sam_publish_app_cmd.rst
Outdated
created as 0.0.1. The app will be created as private by default. SAM CLI prints application created message, metadata | ||
used to create application and link to the console details page. | ||
|
||
>>> sam publish app -t ./packaged.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For integration purposes, it would be cool if we printed a sample CFN resource that a user could copy & paste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, please see comment above: #844 (comment)
designs/sam_publish_app_cmd.rst
Outdated
:depth: 2 | ||
:local: | ||
|
||
``sam publish app`` command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we ever expecting a different type to publish?
If not, perhaps the command structure should be sam app <publish|remove|foo|bar>
This would the main change I would implement for this doc. It would give a more structured feel and would pave the way to richer functionality in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Jacob has commented, the command itself should be an action instead of an entity to remain consistent with other sam commands.
designs/sam_publish_app_cmd.rst
Outdated
|
||
1. SAM specification updates: | ||
|
||
- Add "AWS::ServerlessRepo::Application" sepc in `SAM specification`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo sepc -> spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
designs/sam_publish_app_cmd.rst
Outdated
CodeUri: ./source-code1 | ||
... | ||
|
||
Build Lambda source code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be something sam app publish
could do out of the box. Detect the last build date or diff the prebuild app in $PROJECT_DIR/.aws-sam/build
against the $PROJECT_DIR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope for this command currently. This should be handled when we address this in other commands. Currently no command 'auto builds'.
designs/sam_publish_app_cmd.rst
Outdated
==================================== | ||
|
||
This is the design for a command to publish an application to `AWS Serverless Application Repository (SAR)`_ with a SAM | ||
template. It can be used to create a new application w/ its first version, update existing application's metadata, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be something we could auto suggest, or implement flags for --bump-minor/patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We thought about doing this like npm version, but this is out of scope for publish currently.
designs/sam_publish_app_cmd.rst
Outdated
To publish an application | ||
$ sam publish app -t packaged.yaml --region <region> | ||
|
||
Options: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add the ability to share account IDs or chose to make the app completely public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above: #844 (comment)
Removed the app subcommand from the design. Since we only support Serverless App Repo as the destination for now, |
Closing as released in v0.10.0 🎉 |
@paoptu023 We want this merged. Designs are meant to be apart of the feature and exist to help understand past desicions and what is in or out of scope. Please resubmit the content that was in this PR. I can’t reopen since you deleted the branch. |
Reopen the PR |
Description of changes:
Design doc for adding a new command
sam publish
.Checklist:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.