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.

Sign up for GitHub

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

Jump to bottom

Add --add-host cli option to expose docker container option with the same name. #6078

Merged
merged 13 commits into from
Jan 11, 2024
Merged

Add --add-host cli option to expose docker container option with the same name. #6078

merged 13 commits into from
Jan 11, 2024

Conversation

myukselen
Copy link
Contributor

Which issue(s) does this change fix?

None

Why is this change necessary?

Docker container support add-host cli option to give additional host to IP mappings. This is useful in doing local testing as we can override DNS resolution and replace some client calls with different endpoints.

How does it address the issue?

This change provides --add-host container related option to the SAM cli and it can be used in local invoke, and start api commands.

What side effects does this change have?

There is none I am aware with respect to unit-tests.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

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

Murat Yukselen added 3 commits October 11, 2023 00:46
add: expose Docker --add-host to local invoke command
29357db 
Docker supports providing host to ip settings via add-host cli option. This change exposes this option similarly and utilizes in docker container creation.
lint: formatting long lines
d876cb0
fix: unit tests for add-host cli option
c31bf76 
With these fixes, unit tests are aligned to handle add-host cli option and extra_hosts dictionary for Docker container creation.
@myukselen myukselen requested a review from a team as a code owner October 13, 2023 04:49
@myukselen myukselen requested review from lucashuy and jfuss October 13, 2023 04:49
@github-actions github-actions bot added area/local/start-api sam local start-api command area/local/invoke sam local invoke command area/local/start-invoke pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Oct 13, 2023
@lucashuy
Copy link
Contributor

Thanks for opening this PR! The changes here seem straightforward to me, but I wanted to check in with the team/security first since involves changing the hostname mapping in the containers.

@mndeveci mndeveci added blocked/pending-security-review The PR cannot be merged or reviewed due to pending security review. and removed stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Oct 18, 2023
@DiegoBri11 DiegoBri11 mentioned this pull request Nov 21, 2023
Closed
lucashuy
lucashuy requested changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@lucashuy lucashuy left a comment

Choose a reason for hiding this comment

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

Sorry about the delay in responses, we have approval to go ahead with this change now.

samcli/commands/local/cli_common/invoke_context.py Outdated Show resolved Hide resolved
samcli/commands/local/cli_common/options.py Outdated Show resolved Hide resolved
@lucashuy lucashuy removed the blocked/pending-security-review The PR cannot be merged or reviewed due to pending security review. label Nov 21, 2023
@myukselen
Copy link
Contributor Author

thanks for the response. I will try to address your requests soon with a revision.

Murat Yukselen added 2 commits November 24, 2023 13:24
fix: start-lambda cli is missing add_host for InvokeContext
c7d6feb
refactor: move add_host parsing from InvokeContext
de4cc11 
This change moves add_host parameter parsing into its own type DockerAdditionalHostType .
@myukselen
Copy link
Contributor Author

I made a revision. Please provide any necessary changes to move forward.

@myukselen myukselen requested a review from lucashuy November 24, 2023 18:31
@lucashuy
Copy link
Contributor

Thanks for making the changes! It looks like Github is saying there is a conflict, can you update your local branch to see what's wrong? When that is done, I can start the checks here on Github to run our tests

@myukselen
Copy link
Contributor Author

Hi Lucas, I think I fixed the conflict. Is there anything I can do from my end?

@myukselen
Update lambda_container.py
d2428c6 
deleting the extra line
@mndeveci mndeveci removed the request for review from jfuss December 13, 2023 22:36
@lucashuy
Copy link
Contributor

Hi, it looks like the make command make format should be run so that the linter and formatter we have can fix the files in question. This can be done on your local branch and you can run make pr to validate that things are fixed correctly too.

@myukselen
Copy link
Contributor Author

Hi, sorry for the delays, I am not familiar with Github PR workflows. I pulled my repo branch and ran make format and make pr . The test output still shows this one error I have no idea about:

FAILED tests/unit/cli/test_import_module_proxy.py::TestImportModuleProxy::test_import_should_succeed_for_a_defined_hidden_package_539_pkg_resources_py2_warn - DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html

I just pushed make format changes.

@myukselen
Copy link
Contributor Author

I just pushed make schema changes as I saw this has failed in the workflow.

@jpalomar jpalomar mentioned this pull request Dec 27, 2023
Closed
@HijadelUFO HijadelUFO mentioned this pull request Dec 27, 2023
Open
@dexterinbk dexterinbk mentioned this pull request Dec 27, 2023
Open
This was referenced Jan 5, 2024
Open
Open
lucashuy
lucashuy approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@lucashuy lucashuy left a comment

Choose a reason for hiding this comment

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

Sorry about the delay in response, thanks for taking the time while we sorted out the comments and the CI checks

@lucashuy lucashuy added this pull request to the merge queue Jan 11, 2024
Merged via the queue into aws:develop with commit eabc686 Jan 11, 2024
55 checks passed
@gkuga gkuga mentioned this pull request Mar 31, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucashuy lucashuy lucashuy approved these changes

@mndeveci mndeveci mndeveci approved these changes

Assignees
No one assigned
Labels
area/local/invoke sam local invoke command area/local/start-api sam local start-api command area/local/start-invoke pr/external
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@myukselen @lucashuy @mndeveci @hawflau