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

Refactor Infra, pkg/provider and GKE provider #413

Merged
merged 6 commits into from
Jul 23, 2020

Conversation

geekodour
Copy link
Contributor

@geekodour geekodour commented Jul 19, 2020

Since we're on our way to add KIND support (#390) and EKS support (#384) for different test-infra components, it makes sense to allow the DeploymentVars to be variable among different providers aswell.

This PR adds some minor refactoring and adds the support for DeploymentVars overriding and adds DefaultDeploymentVars aswell. So that the DefaultDeploymentVars can be overridden by -v. This can also be easily extended to add support for provider specific DeploymentVars (#406)

Here are the things that this PR adds:

  • gke.NewGKEClient be called lazily inside infra
  • Remove GKE.setProjectID and add GKE.checkDeploymentVarsAndFiles to validate input DeploymentVars and DeploymentFiles
  • Add support for default DeploymentVars by adding provider.DeploymentResource
  • Add provider.MergeDeploymentVars and tests which is basically the overriding idea
  • Separate DeploymentVars into FlagDeploymentVars and DefaultDeploymentVars

Todo:

  • E2E test of this. (I did partial test, working as expected)

cc : @rajdas98 @krasi-georgiev @nevill @weastel

* Made call to gke.NewGKEClient lazy
* Add GKE.checkDeploymentVarsAndFiles to validate inputs
* Remov GKE.setProjectID

Signed-off-by: Hrishikesh Barman <[email protected]>
* Add support for default DeploymentVars and DeploymentFiles
* Update gke.New
* Update infra to use provider.DeploymentResource

Signed-off-by: Hrishikesh Barman <[email protected]>
* Add test for provider.MergeDeploymentVars
* Change provider.DefaultDeploymentResource to NewDeploymentResource
* Add provider.MergeDeploymentVars
* Separate FlagDeploymentVars and DefaultDeploymentVars

Signed-off-by: Hrishikesh Barman <[email protected]>
@geekodour geekodour changed the title Refactor Infra, pkg/provider and GKE provider [NOTREADY] Refactor Infra, pkg/provider and GKE provider Jul 19, 2020
pkg/provider/gke/gke.go Outdated Show resolved Hide resolved
Signed-off-by: Hrishikesh Barman <[email protected]>
@geekodour geekodour changed the title [NOTREADY] Refactor Infra, pkg/provider and GKE provider Refactor Infra, pkg/provider and GKE provider Jul 19, 2020
Signed-off-by: Hrishikesh Barman <[email protected]>
pkg/provider/gke/gke.go Outdated Show resolved Hide resolved
Copy link
Contributor

@krasi-georgiev krasi-georgiev left a comment

Choose a reason for hiding this comment

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

LGTM with some small nits

infra/infra.go Show resolved Hide resolved
pkg/provider/gke/gke.go Outdated Show resolved Hide resolved
pkg/provider/gke/gke.go Outdated Show resolved Hide resolved
infra/README.md Show resolved Hide resolved
* Update SetupGKEDeploymentResources to SetupDeploymentResources
* Minor refactorings to checkDeploymentVarsAndFiles

Signed-off-by: Hrishikesh Barman <[email protected]>
@geekodour
Copy link
Contributor Author

geekodour commented Jul 21, 2020

Will test this once, will merge if tested. everything seems alright.

@geekodour geekodour merged commit 84dfc08 into prometheus:master Jul 23, 2020
kushalShukla-web pushed a commit to kushalShukla-web/test-infra that referenced this pull request Feb 8, 2025
* Made call to GKE.NewGKEClient lazy
* Remove GKE.setProjectID and add Add GKE.checkDeploymentVarsAndFiles
* Add provider.DeploymentResource with support for default DeploymentVars 
  and FlagDeploymentVars
* Add provider.MergeDeploymentVars and tests
* Add GKE.SetupDeploymentResources which will allow provider specific DeploymentVars

Signed-off-by: Hrishikesh Barman <[email protected]>
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.

2 participants