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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a docker-compose to run server_test in circleci locally #2782

Merged
merged 14 commits into from Oct 14, 2019

Conversation

@gidjin
Copy link
Contributor

commented Oct 8, 2019

Description

Trying to see if we can run server_test in a circleCI container locally

Setup

docker-compose -f docker-compose.circle.yml --compatibility up server_test

NOTE You'll want to run make clean after the above as it uses your local dir as a volume

Code Review Verification Steps

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

References

Screenshots

If this PR makes visible UI changes, an image of the finished UI can help reviewers and casual
observers understand the context of the changes. A before image is optional and
can be included at the submitter's discretion.

Consider using an animated image to show an entire workflow instead of using multiple images. You may want to use GIPHY CAPTURE for this! 馃摳

Please frame screenshots to show enough useful context but also highlight the affected regions.

Copy link
Contributor

left a comment

I saw some stuff that right off the bat we should update. But I think this is a great start for speeding up the cycle of review.

The thing we really need to look into is modifying the docker-compose containers to have the same resource footprint as CircleCI so we can catch stuff. I believe there are --cpu and --mem settings we can use. I think its a percentage of available resources. If that's the case then let's use the specs from one of our machines as the baseline (and comment it somewhere) and then set the percentage accordingly.

@gidjin gidjin force-pushed the gdjn_run_circle_ci_container_locally branch from 7afbb4f to 0cbf1e1 Oct 9, 2019
@gidjin gidjin force-pushed the gdjn_run_circle_ci_container_locally branch from 0cbf1e1 to ede075d Oct 9, 2019
@codecov

This comment has been minimized.

Copy link

commented Oct 9, 2019

Codecov Report

Merging #2782 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #2782   +/-   ##
======================================
  Coverage    58.2%   58.2%           
======================================
  Files         291     291           
  Lines       12755   12755           
======================================
  Hits         7412    7412           
  Misses       4593    4593           
  Partials      750     750
Impacted Files Coverage 螖
pkg/cli/migration_path.go 40% <100%> (酶) 猬嗭笍
@gidjin gidjin force-pushed the gdjn_run_circle_ci_container_locally branch from 4852dfc to c3eeb25 Oct 9, 2019
@gidjin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

I've updated the PR with the changes you noted and the configuration. The test does fail when I run it locally, can you confirm if that is what you see as well.

@gidjin gidjin marked this pull request as ready for review Oct 9, 2019
Copy link
Contributor

left a comment

I'm going to muck with this a bit. I also saw it fail but we're pretty close here. The utility is great and I'd like to see if I can't explore a bit before I approve.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

I've updated the PR with the changes you noted and the configuration. The test does fail when I run it locally, can you confirm if that is what you see as well.

What was missing was a bunch of environment variables. But once I sorted that bit I got things passing locally. Fortunately I also noticed that we could get some test failures of the exact same sort we're seeing in CircleCI with your branch. So we're one track.

Additionally, I found out we aren't doing the right thing with our env vars for these tests anyway so I think I've updated it correctly in CircleCI. I'll keep an eye on it. I may also decrease the resources for this docker-compose file of yours and see what that does.

resources:
limits:
cpus: '2.0'
memory: 4096M

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Oct 11, 2019

Contributor

I decreased this a bit more to see if it would change anything. I'm still getting flaky tests but I can't nail it down exactly.

@gidjin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2019

I checked out the changes look good, though that's a lot of environment variables. I ran the test a couple of times locally and got one passing and one failing run, so I agree we have a way to replicate the issue. I did try the following but still had a pass and a fail so I don't think it's specific to the image.

   server_test:
-    image: trussworks/circleci-docker-primary:e5c10bf19185aa55354901106bad3ffd4c016265
+    #image: trussworks/circleci-docker-primary:e5c10bf19185aa55354901106bad3ffd4c016265
+    image: golang:1.12
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

I checked out the changes look good, though that's a lot of environment variables.

Yeah, the server_tests in the pkg/cli/ directory should really be more complete. But for now they just read from the environment directly and do checks. We can fix that but I don't think anyone has had the time to make it a priority.

I ran the test a couple of times locally and got one passing and one failing run, so I agree we have a way to replicate the issue.

I agree. This is good work!

I did try the following but still had a pass and a fail so I don't think it's specific to the image.

   server_test:
-    image: trussworks/circleci-docker-primary:e5c10bf19185aa55354901106bad3ffd4c016265
+    #image: trussworks/circleci-docker-primary:e5c10bf19185aa55354901106bad3ffd4c016265
+    image: golang:1.12

That's good to know. I'd wait on merging this until the go1.13 PR gets merged in and then let's update the primary image here too.

Copy link
Contributor

left a comment

馃殌 - Wait until the new go1.13 image is available and then update and merge this in!

@gidjin gidjin merged commit 3ec0c38 into master Oct 14, 2019
18 checks passed
18 checks passed
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_storybook_app Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: check_generated_code Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch/go 100% of diff hit (target 57.9%)
Details
codecov/project/go 57.9% (+0%) compared to fc536f3
Details
@gidjin gidjin deleted the gdjn_run_circle_ci_container_locally branch Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can鈥檛 perform that action at this time.