-
Notifications
You must be signed in to change notification settings - Fork 4.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
Support --s3-endpoint-url
argument to cloudformation package
#3309
Support --s3-endpoint-url
argument to cloudformation package
#3309
Conversation
434a276
to
7c532ba
Compare
Codecov Report
@@ Coverage Diff @@
## develop #3309 +/- ##
========================================
Coverage 93.96% 93.96%
========================================
Files 188 188
Lines 14525 14525
========================================
Hits 13649 13649
Misses 876 876
Continue to review full report at Codecov.
|
@baronswindle great work! However, there is some work required to support |
Any news regarding this issue? |
@miron4dev With renewed interest are there still plans to work on this or is localstack going to be deprecating aws cli v1 anytime soon? |
Would love to see this feature added to aws cli v1. Can you please elaborate which changes are still required @rafenden @codygman ? (Apart from fixing the merge conflicts). I'd be happy to contribute, if needed, unless @baronswindle can update the PR to push this over the line..? Thanks |
7c532ba
to
4085282
Compare
I just has this problem. is there any update? |
4085282
to
e8693eb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3309 +/- ##
========================================
Coverage 93.98% 93.98%
========================================
Files 188 188
Lines 14560 14560
========================================
Hits 13684 13684
Misses 876 876 ☔ View full report in Codecov by Sentry. |
@@ -276,7 +286,8 @@ def _run_main(self, parsed_args, parsed_globals): | |||
"s3", | |||
config=Config(signature_version='s3v4'), | |||
region_name=parsed_globals.region, | |||
verify=parsed_globals.verify_ssl) | |||
verify=parsed_globals.verify_ssl |
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.
@baronswindle Looks like we're missing a comma at the end of this line. Seems to be breaking the builds currently:
ERROR: Failure: SyntaxError (invalid syntax (deploy.py, line 290))
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/virtualenv/python2.7.14/lib/python2.7/site-packages/nose/loader.py", line 418, in loadTestsFromName
addr.filename, addr.module)
File "/home/travis/virtualenv/python2.7.14/lib/python2.7/site-packages/nose/importer.py", line 47, in importFromPath
return self.importFromDir(dir_path, fqname)
File "/home/travis/virtualenv/python2.7.14/lib/python2.7/site-packages/nose/importer.py", line 94, in importFromDir
mod = load_module(part_fqname, fh, filename, desc)
File "/home/travis/build/aws/aws-cli/tests/unit/customizations/cloudformation/test_deploy.py", line 20, in <module>
from awscli.customizations.cloudformation.deploy import DeployCommand
File "/home/travis/build/aws/aws-cli/awscli/customizations/cloudformation/__init__.py", line 14, in <module>
from awscli.customizations.cloudformation.deploy import DeployCommand
File "/home/travis/build/aws/aws-cli/awscli/customizations/cloudformation/deploy.py", line 290
endpoint_url=parsed_args.s3_endpoint_url)
^
SyntaxError: invalid syntax
Thanks!
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.
@whummer My apologies - that was careless. Just pushed up the fix.
e8693eb
to
d0bf26a
Compare
Builds are green now, thanks @baronswindle . Are we good to get this merged from your side @rafenden @kyleknap @jamesls ? Please let us know if anything is missing - would be great to get this feature in. Thanks! |
Awesome, thanks for approving the PR @eitens-mak ! Do you know what is the further process for getting this merged? Could one of the project maintainers please help us out here? Many thanks, really appreciated!! |
Trying to bump this PR one more time - apologies for being persistent, but it would be great to get an official statement from the maintainers what is still required to get this merged and released. @eitens-mak @stealthycoin @kyleknap @nateprewitt Can you please help out here? Many thanks in advance. |
@whummer Hey, I'm not a maintainer. Just a random internet person interested in merging this. I'm surprised Github let me approve it, but I was mostly hoping to get some traction because I too want this. |
@jamesls @kyleknap @bisdavid @JordonPhillips @stealthycoin Can we please get some help or official statement for this PR? (Pinging some of the top contributors, as I wasn't able to figure out the list of official maintainers of this repository - apologies for the broadcast.) Would be great if we could get an official 👍 / 👎 on this pull request - are there any concerns about this new feature, anything still missing (as per the contribution guidelines)? Many thanks for your help. |
@baronswindle @eitens-mak @wh1tecat-nya @codygman @miron4dev @rafenden We took the liberty to incorporate this change directly into the You can try installing the latest version of the
The
Hope that helps - any feedback welcome. Thanks for your help. |
Can this be merged yet? I desperatley need to be able to deploy our cloudformation stack on Localstack and I need to use |
I'm also really hoping this can be merged soon. |
Any progress on this? |
3 years laters.... Can we get this merged please? |
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.
Changes LGTM (FWIW, as I'm not an official maintainer of this repo) - we've been using this approach successfully in a fork of this repo providing the awslocal
command line.
Would also love to see this getting merged, so we don't have to depend on maintaining forks for customizing the S3 CloudFormation endpoint URL. Thanks in advance!
@kyleknap are you authorized to merge? |
bump. Any communication from aws team would be appreciated 👌 |
Hi everyone, Thank you for your patience, and I apologize that it's taken so long to get this reviewed. We're making some changes to our community contribution process to provide more transparency and make community contributions successful. This PR linked should be reviewed within the next couple weeks as we work through all open pull requests to see where the fall in our newly proposed contribution process. You can read more about the changes to the contribution process here. |
@@ -60,6 +60,7 @@ def setUp(self): | |||
fail_on_empty_changeset=True, | |||
s3_bucket=None, | |||
s3_prefix="some prefix", | |||
s3_endpoint_url="http://localhost", |
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.
I would like to see tests explicit tests for this behavior.
At the least there needs to be :
- Test where argument is not supplied showing that the old endpoint behavior used.
- If we do supply an override argument, that is correctly passed through and used in the request.
Putting this PR in draft status as we haven't heard back since last year regarding the requested changes. |
Hi @baronswindle just wanted to check in on this PR — is adding the tests requested here something that you are able to address? #3309 (comment) |
Closing this PR as we have not heard back regarding the requested changes. |
Just wanted to follow up and share this update on the linked issue:
|
Most other S3 commands accept an
--endpoint-url
argument.cloudformation package
uses S3 under the hood, so it made sense to me that we could allow users to configure the S3 endpoint.Additionally, it makes it somewhat easier to use to launch CF stacks for local development/testing. See localstack/localstack#632.