Skip to content
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

[WIP] sam publish app design doc #819

Closed
wants to merge 4 commits into from
Closed

[WIP] sam publish app design doc #819

wants to merge 4 commits into from

Conversation

paoptu023
Copy link
Contributor

Issue #, if available:
N/A

Description of changes:
Design doc for sam publish app command

Checklist:

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

@paoptu023 paoptu023 requested review from sanathkr, jfuss and sriram-mv and removed request for sanathkr November 30, 2018 22:04
designs/sam_publish_app_cmd.rst Outdated Show resolved Hide resolved
designs/sam_publish_app_cmd.rst Outdated Show resolved Hide resolved
designs/sam_publish_app_cmd.rst Outdated Show resolved Hide resolved
designs/sam_publish_app_cmd.rst Show resolved Hide resolved
designs/sam_publish_app_cmd.rst Outdated Show resolved Hide resolved

1. Add a new top-level command called ``sam publish app`` with the following options.

-t, --template PATH AWS SAM template to publish.
Copy link
Contributor

Choose a reason for hiding this comment

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

Any defaults to this?

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 can make the default template path the same as other sam commands, what would be a good value?

designs/sam_publish_app_cmd.rst Outdated Show resolved Hide resolved
*Explain how this feature will be implemented. Highlight the components of your implementation, relationships*
*between components, constraints, etc.*

SAM CLI will read the packaged SAM template and pass it as string to `aws-serverlessrepo-python <https://github.com/awslabs/aws-serverlessrepo-python>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't exist.

Adding another dependency isn't trivial and has lots of questions:

  • How do we keep SAM CLI updated to the latest (or recently latest versions)?
  • What python versions does this support?
  • Will this library support all new versions?
  • Is there an SLA for supporting new python versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I just made it public. We don't expect this library to be frequently updated, and we will make sure that SAM CLI gets updated to the latest version following any required processes.

The library supports the same versions that SAM CLI support: https://github.com/awslabs/aws-serverlessrepo-python/blob/master/setup.py#L50. I use Tox to test against different Python versions and will set up integration with Travis to run tests for every code changes.

We haven't considered about supporting new Python versions or the SLA, happy to discuss the details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

designs/sam_publish_app_cmd.rst Outdated Show resolved Hide resolved

Out-of-Scope
------------
#. Create new application or version if the ``--meta-data`` option is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is only to update an application? If this isn't being used to publishing, why it is an option for the sam publish app command?

What is the reason this isn't supported with publishing an app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In SAR, application version and metadata are separate entities, so updating the metadata doesn't create a new version. Initially we thought about just using sam publish app to update metadata and publish app, but there are some fields that can't be modified after the app/version is created which makes it complicated.

For example, license can't be changed after an app is created, if customers provide a different license, should we ignore it or fail the command? Ignoring the new license value may make customers feel that it has been updated because the publish command succeeds. Failing the command would require them to provide different metadata spec.

Considering the above, we decided to separate updating metadata from publishing app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR to support updating metadata with publishing and returning details if you'd like to take a look: amazon-archives/aws-serverlessrepo-python#14.

@jfuss jfuss self-assigned this Dec 4, 2018
@jfuss
Copy link
Contributor

jfuss commented Dec 7, 2018

@paoptu023 Please fork the repo and work from that instead of pushing work to the main repo. This is called out in our CONTRIBUTING.md in the "To send us a pull request, please:" section.

@paoptu023
Copy link
Contributor Author

Closing in favor of #844.

@paoptu023 paoptu023 closed this Dec 7, 2018
@paoptu023 paoptu023 deleted the qwang-patch branch December 7, 2018 18:27
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.

2 participants