Skip to content

Terraform removal from infrastructure reconciler#596

Closed
elchead wants to merge 1 commit into
gardener:masterfrom
elchead:terraform-rm
Closed

Terraform removal from infrastructure reconciler#596
elchead wants to merge 1 commit into
gardener:masterfrom
elchead:terraform-rm

Conversation

@elchead

@elchead elchead commented Nov 14, 2022

Copy link
Copy Markdown
Contributor

How to categorize this PR?

/area control-plane
/kind enhancement
/platform azure

What this PR does / why we need it:
Reconciliation of infrastructure resources are managed directly with a graph flow and using Azure SDK client API instead of using Terraformer.

Which issue(s) this PR fixes:
Fixes #411

Special notes for your reviewer:

Release note:


@ghost ghost added area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension platform/azure Microsoft Azure platform/infrastructure labels Nov 14, 2022
@ghost

ghost commented Nov 14, 2022

Copy link
Copy Markdown

@elchead Thank you for your contribution.

@ghost ghost added the needs/rebase Needs git rebase label Nov 14, 2022
@ghost

ghost commented Nov 14, 2022

Copy link
Copy Markdown

@elchead You need rebase this pull request with latest master branch. Please check.

@ghost ghost added needs/review Needs review size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs/second-opinion Needs second review by someone else labels Nov 14, 2022
@gardener-robot-ci-2

Copy link
Copy Markdown
Contributor

Thank you @elchead for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

Comment thread .test-defs/infrastructure-test.yaml Outdated
Comment thread pkg/apis/azure/validation/infrastructure.go Outdated
Comment thread pkg/azure/client/helper.go
Comment thread pkg/azure/client/types.go
Comment thread pkg/azure/client/types.go Outdated
Comment thread pkg/controller/infrastructure/actuator_delete.go Outdated
)

// MakeCluster returns a cluster object used for testing.
func MakeCluster(pods, services string, region string, countFaultDomain, countUpdateDomain int32) *controller.Cluster {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's not group testing code with production code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree its suboptimal, but its an internal package. Should i create an entirely new test package that can be shared across different pkg tests?
E.g. pkg/internal/infrastructure/test ?

Comment thread pkg/internal/infrastructure/terraform.go Outdated
Comment on lines +136 to +146
// TODO copy from AWS PR (use taskBuilder component to share?)
// addTask adds a wrapped task for the given task function and options.
func (f FlowReconciler) addTask(g *flow.Graph, name string, fn flow.TaskFn, options ...shared.TaskOption) flow.TaskIDer {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you can use the "shared" library version for that now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Martin's flowContext has a persistor which I don't need and I cannot opt out. Since it's only very few code and since the libraries are not synced at the moment at any rate, I would defer the refactor to opt out of the persistor until the libraries are really shared

Comment thread pkg/controller/infrastructure/infraflow/azreconciler.go Outdated
@elchead elchead marked this pull request as ready for review March 9, 2023 14:55
@elchead elchead requested review from a team as code owners March 9, 2023 14:55
@kon-angelo

Copy link
Copy Markdown
Contributor

/test

@testmachinery

testmachinery Bot commented Mar 9, 2023

Copy link
Copy Markdown

Testrun: e2e-q88m8
Workflow: e2e-q88m8-wf
Phase: Succeeded

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| infrastructure-test | infrastructure-test | Succeeded | 33m38s   |
| bastion-test        | bastion-test        | Succeeded | 11m3s    |
+---------------------+---------------------+-----------+----------+

@kon-angelo

Copy link
Copy Markdown
Contributor

/reviewed ok-to-test

@ghost ghost added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 9, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 9, 2023
@kon-angelo

Copy link
Copy Markdown
Contributor

/test

@testmachinery

testmachinery Bot commented Mar 10, 2023

Copy link
Copy Markdown

Testrun: e2e-b4fz7
Workflow: e2e-b4fz7-wf
Phase: Failed

+--------------------------+--------------------------+-----------+----------+
|           NAME           |           STEP           |   PHASE   | DURATION |
+--------------------------+--------------------------+-----------+----------+
| infrastructure-test      | infrastructure-test      | Succeeded | 35m6s    |
| infrastructure-test-flow | infrastructure-test-flow | Failed    | 5m20s    |
| bastion-test             | bastion-test             | Succeeded | 10m30s   |
+--------------------------+--------------------------+-----------+----------+

@kon-angelo

Copy link
Copy Markdown
Contributor

/test

@testmachinery

testmachinery Bot commented Mar 10, 2023

Copy link
Copy Markdown

Testrun: e2e-zcmzw
Workflow: e2e-zcmzw-wf
Phase: Failed

+--------------------------+--------------------------+--------+----------+
|           NAME           |           STEP           | PHASE  | DURATION |
+--------------------------+--------------------------+--------+----------+
| infrastructure-test      | infrastructure-test      | Failed | 42m35s   |
| infrastructure-test-flow | infrastructure-test-flow | Failed | 1h5m53s  |
| bastion-test             | bastion-test             | Failed | 12m27s   |
+--------------------------+--------------------------+--------+----------+

@ghost ghost added the kind/api-change API change with impact on API users label Mar 10, 2023
@elchead elchead force-pushed the terraform-rm branch 2 times, most recently from d50207a to 5a7f047 Compare March 10, 2023 19:30
@elchead

elchead commented Jun 15, 2023

Copy link
Copy Markdown
Contributor Author

@kon-angelo can you mention the PR when the feature gets merged?

@elchead elchead closed this Jun 15, 2023
@ghost ghost added the status/closed Issue is closed (either delivered or triaged) label Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/control-plane Control plane related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase needs/review Needs review needs/second-opinion Needs second review by someone else platform/azure Microsoft Azure platform/infrastructure size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/closed Issue is closed (either delivered or triaged)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Terraform removal for infrastructure controller

4 participants