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

Removes special treatment of GCP authentication #465

Merged
merged 2 commits into from
May 22, 2019

Conversation

benlangfeld
Copy link
Contributor

What are you trying to accomplish with this PR?

Simplify code related to GCP authentication.

How is this accomplished?

Upgrading to a version of kubeclient which deals with GCP auth natively: ManageIQ/kubeclient#394 (comment)

What could go wrong?

GCP auth could break. I'm not in a position to test it, but our unit tests were passing before they were removed.

Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

GCP auth could break. I'm not in a position to test it, but our unit tests were passing before they were removed.

We have a way to easily test a specific commit using a test app in a GCP cluster. We'll do that after this round of feedback is addressed. :)

lib/kubernetes-deploy/kubeclient_builder/kube_config.rb Outdated Show resolved Hide resolved
end

def teardown
WebMock.allow_net_connect!
end

def test_auth_use_default_gcp_success
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test invalid now, or are you removing it because we no longer own the code that makes it work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it because it’s no longer our responsibility, the same way we don’t have test coverage for OIDC for example.

Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

I had to push this as a commit on the Shopify copy of the repo (here) to use our test mechanism, but was then able to run the test without issue. Employees can confirm here that the test deploy ran without issue based on the correct sha: Using kubernetes-deploy 0.26.4 from https://github.com/Shopify/kubernetes-deploy.git (at 54f7cd2@54f7cd2)

@KnVerey
Copy link
Contributor

KnVerey commented May 22, 2019

@timothysmith0609 can you please 👀 this?

@timothysmith0609
Copy link
Contributor

The diff makes sense to me, in the end we've just moved the same logic out to the kubeclient gem. Since the test run has passed I feel pretty confident.

@KnVerey KnVerey merged commit 297010d into Shopify:master May 22, 2019
@benlangfeld benlangfeld deleted the google-auth branch July 2, 2019 15:37
@RyanBrushett RyanBrushett temporarily deployed to rubygems July 3, 2019 18:20 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants