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

feat: support env vars and secrets for sidecars #1725

Merged
merged 5 commits into from
Nov 30, 2020
Merged

feat: support env vars and secrets for sidecars #1725

merged 5 commits into from
Nov 30, 2020

Conversation

iamhopaul123
Copy link
Contributor

fixes #1119

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@iamhopaul123 iamhopaul123 requested a review from a team as a code owner November 25, 2020 19:57
@iamhopaul123 iamhopaul123 added the WIP Pull requests that are being modified now. label Nov 25, 2020
@iamhopaul123 iamhopaul123 force-pushed the sidecar/add-var-secret branch from 7c986cf to 786c8c0 Compare November 25, 2020 20:37
@iamhopaul123 iamhopaul123 removed the WIP Pull requests that are being modified now. label Nov 25, 2020
@iamhopaul123 iamhopaul123 force-pushed the sidecar/add-var-secret branch from 786c8c0 to e5c323f Compare November 25, 2020 20:37
@iamhopaul123 iamhopaul123 force-pushed the sidecar/add-var-secret branch from e5c323f to 1a5c46e Compare November 25, 2020 20:48
huanjani
huanjani reviewed Nov 25, 2020
View reviewed changes
e2e/sidecars/sidecars_test.go
logging:
destination:
Name: cloudwatch
region: us-west-2
region: us-east-1
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just curious why you changed regions here...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question it is because our e2e tests are deployed in us-east-1, so that firelens knows where to find the log group and create the log stream in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but i do think we need to update the e2e test so that this error can be caught.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok now it should be able to catch it if the firelens is not able to create the log stream in the log group.

efekarakus
efekarakus approved these changes Nov 30, 2020
View reviewed changes
Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

Awesome thanks!

@efekarakus efekarakus changed the title feat(sidecar): add env vars and secrets feat(sidecar): add env vars and secrets support Nov 30, 2020
@efekarakus efekarakus changed the title feat(sidecar): add env vars and secrets support feat: support env vars and secrets for sidecars Nov 30, 2020
@iamhopaul123 iamhopaul123 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 30, 2020
@iamhopaul123 iamhopaul123 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 30, 2020
iamhopaul123
iamhopaul123 commented Nov 30, 2020
View reviewed changes
e2e/sidecars/nginx/Dockerfile
@@ -1,2 +1,8 @@
FROM nginx
COPY nginx.conf /etc/nginx/nginx.conf
FROM nginx:1.15-alpine
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://serverfault.com/questions/577370/how-can-i-use-environment-variables-in-nginx-conf

@mergify mergify bot merged commit d709cfe into aws:mainline Nov 30, 2020
kahirul
kahirul reviewed Dec 28, 2020
View reviewed changes
e2e/sidecars/sidecars_test.go
@@ -49,14 +50,18 @@ sidecars:
nginx:
port: 80
image: %s # Image URL for sidecar container.
variables:
NGINX_PORT: %s
Copy link
Contributor

@kahirul kahirul Dec 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is it possible to set variables or secrets per-environment, i.e. dev and prod?

Copy link
Contributor Author

@iamhopaul123 iamhopaul123 Dec 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I think you can do the override like this

sidecars:
  nginx:
    port: 80
    image: %s    # Image URL for sidecar container.
    variables:
      NGINX_PORT: 80

environments:
  dev:
    sidecars:
      nginx:
        variables:
          NGINX_PORT: 8080
  prod:
    sidecars:
      nginx:
        variables:
          NGINX_PORT: 5000

thrau pushed a commit to localstack/copilot-cli-local that referenced this pull request Dec 9, 2022
@iamhopaul123 @thrau
feat: support env vars and secrets for sidecars (aws#1725)
f783cd8 
<!-- Provide summary of changes -->
fixes aws#1119
<!-- Issue number, if available. E.g. "Fixes aws#31", "Addresses aws#42, 77" -->

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kahirul kahirul kahirul left review comments

@efekarakus efekarakus efekarakus approved these changes

@huanjani huanjani huanjani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Environment Variable and Secret for Sidecar container
4 participants
@iamhopaul123 @kahirul @efekarakus @huanjani