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

Remove TSP App and Public API flags #2788

Merged
merged 8 commits into from Oct 9, 2019

Conversation

@chrisgilmerproj
Copy link
Contributor

commented Oct 8, 2019

Description

There is no Public API endpoint anymore so I've removed the flags around serving that endpoint.

There is also no TSP app anymore so I've gone ahead and removed the remaining bits so we can disable route 53 DNS.

Reviewer Notes

Are there things that I need to be aware of before removing the TSP app? I think its fairly trivial to bring back and it reduces our attack surface area on the app. But if I've missed a product decision I want to be aware of it.

Setup

No setup needed, just watch the tests pass.

Code Review Verification Steps

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

References

@chrisgilmerproj chrisgilmerproj self-assigned this Oct 8, 2019
@codecov

This comment has been minimized.

Copy link

commented Oct 8, 2019

Codecov Report

Merging #2788 into master will increase coverage by 0.1%.
The diff coverage is 33.4%.

@@           Coverage Diff            @@
##           master   #2788     +/-   ##
========================================
+ Coverage    57.8%   57.8%   +0.1%     
========================================
  Files         278     278             
  Lines       12648   12624     -24     
========================================
- Hits         7303    7293     -10     
+ Misses       4596    4582     -14     
  Partials      749     749
Impacted Files Coverage Δ
pkg/cli/hosts.go 74% <ø> (-2%) ⬇️
pkg/auth/authentication/auth.go 40.3% <ø> (+0.3%) ⬆️
pkg/cli/auth.go 66.2% <ø> (-1.1%) ⬇️
pkg/auth/session.go 50% <ø> (-3.1%) ⬇️
pkg/auth/authentication/devlocal.go 35.6% <ø> (ø) ⬆️
pkg/cli/devlocal.go 15.4% <ø> (+2.5%) ⬆️
pkg/auth/authentication/login_gov.go 18.9% <0%> (+1.2%) ⬆️
pkg/auth/cookie.go 79.7% <100%> (-0.5%) ⬇️
@chrisgilmerproj chrisgilmerproj marked this pull request as ready for review Oct 8, 2019
Copy link
Contributor

left a comment

Other than the one change everything else looks good to me. Would prefer to have a second review to be safe and this PR touches a lot of things.

@@ -14,7 +14,7 @@ services:
milmove_migrate:
depends_on:
- database
image: 923914045601.dkr.ecr.us-west-2.amazonaws.com/app-migrations:git-branch-placeholder_branch_name
image: 923914045601.dkr.ecr.us-west-2.amazonaws.com/app-migrations:git-branch-master

This comment has been minimized.

Copy link
@mr337

mr337 Oct 8, 2019

Contributor

Should this be reverted? Not seeing connection between this and the removal of TSP.

@chrisgilmerproj chrisgilmerproj requested a review from mr337 Oct 8, 2019
Copy link
Contributor

left a comment

Looks like all of this works as expected for me. :shipit:

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

There's a failing server test that is unrelated to the changes. I'm trying to track it down but so far haven't been able to fix it. These same tests pass elsewhere and revolve around testing TLS connection with the fake certs we provide.

@chrisgilmerproj chrisgilmerproj referenced this pull request Oct 9, 2019
@chrisgilmerproj chrisgilmerproj dismissed mr337’s stale review Oct 9, 2019

Resolved.

@chrisgilmerproj chrisgilmerproj merged commit f5f3724 into master Oct 9, 2019
17 of 18 checks passed
17 of 18 checks passed
codecov/patch/go 33.4% of diff hit (target 57.6%)
Details
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/project/go 57.6% (+0.1%) compared to 2f6f3c8
Details
@chrisgilmerproj chrisgilmerproj deleted the cg_remove_tsp_app_and_public_api branch Oct 9, 2019
@lynzt
lynzt approved these changes Oct 9, 2019
Copy link
Contributor

left a comment

Looks good to me... Just a few random questions. 🚢

# Add verbose (-v) so go-junit-report can parse it for CircleCI results
DB_NAME=$(DB_NAME_TEST) DB_PORT=$(DB_PORT_TEST) go test -v -parallel 8 -count 1 -short $$(go list ./... | grep -v \\/pkg\\/gen\\/ | grep -v \\/cmd\\/ | grep -v mocks)
DB_NAME=$(DB_NAME_TEST) DB_PORT=$(DB_PORT_TEST) go test -v -parallel 4 -count 1 -short $$(go list ./... | grep -v \\/pkg\\/gen\\/ | grep -v \\/cmd\\/ | grep -v mocks)

This comment has been minimized.

Copy link
@lynzt

lynzt Oct 9, 2019

Contributor

just curious why 4 vs 8...

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Oct 11, 2019

Author Contributor

Trying to turn down the resource load on the containers. There's some magic number here that I don't understand yet but felt like 8 was too high.

@@ -553,7 +551,6 @@ endif
db_test_start: ## Start Test DB
ifndef CIRCLECI
brew services stop postgresql 2> /dev/null || true
endif
@echo "Starting the ${DB_DOCKER_CONTAINER_TEST} docker database container..."

This comment has been minimized.

Copy link
@lynzt

lynzt Oct 9, 2019

Contributor

so this is done via circle now?

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Oct 11, 2019

Author Contributor

Circle always handled the DBs for the server tests. But not for the integration tests. I think when we moved to docker-compose for the integration tests I forgot to clean this one up. But previously this step was running and failing and then continuing on for all server tests. So I went ahead and cut out the extra time it was spending on this part of the code.

This comment has been minimized.

Copy link
@lynzt

lynzt Oct 11, 2019

Contributor

cool, thanks for that context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.