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-10464 Add reasons for why local docker setup isn't supported anymore #89

Conversation

felipe-lee
Copy link
Contributor

Description

In a previous PR (#74) the local docker setup broke and in a later PR (#78) the instructions were updated indicating the local docker setup wouldn't be supported anymore, but didn't give reasoning. This PR adds some reasons for the change so that future maintainers can have some context.

Reviewer Notes

Given that the local docker setup is broken, would it maybe be better to delete the instructions and have my git commit list the reasons?

Setup

Easiest way to read the formatted README would probably be to look at the README on this branch in github.

Code Review Verification Steps

  • Have the Jira acceptance criteria been met for this change?

References

README.md Outdated
@@ -182,8 +182,17 @@ entirely at a future date.

#### Alternative Setup: Docker

It is also possible to run load tests from within a Docker container, eliminating the need to set up a valid python
environment. This requires Docker and `docker-compose` to be installed on your machine. Get them here:
It might be possible to run load tests from within a docker container, though given recent updates this seems to not be
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe consider dropping "recent" since some time might pass before this is looked at again. Also, it might be worth linking to the PRs that introduced the breaking changes here

@pearl-truss
Copy link
Contributor

Given that the local docker setup is broken, would it maybe be better to delete the instructions and have my git commit list the reasons?

I'm in favor of this since it's not working I don't think we need instructions for it anymore but it would be nice to have a reference to the reasons

@ronaktruss
Copy link
Contributor

I'm also in favor of removing the instructions and indicating the reasons that we don't support it. If we do this we should also remove some of the code/files that are specific for this functionality such as local_locust.py and docker-compose.local.yaml.

@felipe-lee
Copy link
Contributor Author

Sounds good, I can make those updates, thanks for the feedback!

Add a more detailed reason for removing `asdf` setup.
Add links to PRs that removed `asdf` and `docker` setups.
@felipe-lee
Copy link
Contributor Author

@pearl-truss @ronaktruss , I deleted the local docker setup entirely now, added the PR links to the README, and modified the details a little.

I'd mentioned in my description that maybe we should just remove everything from the README regarding local docker asdf, but now that I think about it, having the "Unsupported Setup" section might be good to keep so that as we hand this off to other teams. It's more visible why we don't have more setup options rather than having to dig through the commit history 🤔

@pearl-truss pearl-truss self-requested a review December 6, 2021 21:24
Copy link
Contributor

@pearl-truss pearl-truss left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@felipe-lee felipe-lee merged commit 017c61e into master Dec 7, 2021
@felipe-lee felipe-lee deleted the MB-10464-update-docs-to-indicate-why-docker-is-no-longer-supported-locally branch December 7, 2021 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Team Created by A-Team
3 participants