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

MB-9638: Add ecs deploy + codebuild config + nix updates #74

Merged
merged 4 commits into from
Oct 13, 2021

Conversation

ahobson
Copy link
Contributor

@ahobson ahobson commented Sep 2, 2021

Description

Provide a way to deploy to ECS

Reviewer Notes

Is there anything you would like reviewers to give additional scrutiny?

Setup

Add any steps or code to run in this section to help others prepare to run your code:

echo "Code goes here"

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Jira acceptance criteria been met for this change?

References

@ahobson ahobson marked this pull request as ready for review September 28, 2021 20:20
@ahobson ahobson changed the title Add ecs deploy + codebuild config + nix updates MB-9638: Add ecs deploy + codebuild config + nix updates Oct 4, 2021
@ahobson ahobson requested review from gidjin and removed request for gidjin and sandy-wright October 4, 2021 18:02
@ronaktruss ronaktruss assigned ronaktruss and unassigned ronaktruss Oct 12, 2021
@ronaktruss ronaktruss self-requested a review October 12, 2021 22:16
Comment on lines +14 to +17
COPY locustfiles /app/locustfiles
COPY static /app/static
COPY tasks /app/tasks
COPY utils /app/utils
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that the original line was replaced with this? When using docker to run tests locally with this change we get an error saying can't open file '/app/local_locust.py'. We may need add something like

COPY local_locust.py /app/local_locust.py

Both the old and new code work when running locust in AWS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason we need to run the load test inside docker locally? It has caused some complication/problems in the past

Copy link
Contributor

@ronaktruss ronaktruss Oct 13, 2021

Choose a reason for hiding this comment

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

Moving forward there shouldn't be as we would likely want all of the tests run in AWS. I mentioned the above because up until this PR local testing was the main method of testing. The setup docs provide instructions to support local testing through docker so thought we were going to keep supporting it.

If its caused some issues in the past and we aren't planning on using it moving forward I don't think this is something we need to address. We will likely want to update the docs to reflect 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.

Ah! Yeah, running in docker locally caused problems in the past, and so I think we'd like to not do that anymore. Perhaps that can be a follow up PR to cleanup/remove the documentation to make that clearer?

@ronaktruss
Copy link
Contributor

Outside of the above comment, everything else looks good to me!

@ronaktruss ronaktruss mentioned this pull request Oct 13, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants