Skip to content

Refactor config and substitution#41

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
Danil-Grigorev:refactor-config-and-substitution
May 18, 2021
Merged

Refactor config and substitution#41
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
Danil-Grigorev:refactor-config-and-substitution

Conversation

@Danil-Grigorev
Copy link
Copy Markdown

@Danil-Grigorev Danil-Grigorev commented May 17, 2021 •
edited
Loading

This change separates substitution, config and cloud methods in their own packages in
preparation to support bootstrap implementation folowing in #42.

It refactors testing and adding full test coverage for moved methods. This is required for later bootstrap manifests implementation.

Commits with actual changes https://github.com/openshift/cluster-cloud-controller-manager-operator/pull/41/files/600422dafca2e1c6cc63068a0061fc52f034366f..HEAD

Follow up on #40

@openshift-ci openshift-ci Bot requested review from Fedosin and JoelSpeed May 17, 2021 09:50
This was referenced May 17, 2021
Merged
Merged
@Danil-Grigorev Danil-Grigorev force-pushed the refactor-config-and-substitution branch from af9f217 to 4d25c97 Compare May 17, 2021 16:28
lobziik
lobziik reviewed May 17, 2021
View reviewed changes
Comment thread pkg/cloud/openstack/openstack.go
lobziik
lobziik reviewed May 17, 2021
View reviewed changes
Comment thread pkg/config/config_test.go Outdated
@Danil-Grigorev Danil-Grigorev force-pushed the refactor-config-and-substitution branch 3 times, most recently from fa1cfd2 to b7db340 Compare May 17, 2021 18:31
@Danil-Grigorev Danil-Grigorev requested a review from lobziik May 17, 2021 18:31
elmiko
elmiko reviewed May 17, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

nice work Danil, looks generally good to me but i have a couple style suggestions and a few questions.

Comment thread pkg/cloud/aws/aws.go Outdated
Comment thread pkg/cloud/openstack/openstack.go Outdated
Comment thread pkg/config/config.go Outdated
@Danil-Grigorev Danil-Grigorev force-pushed the refactor-config-and-substitution branch 2 times, most recently from 6ba8428 to 15ec1df Compare May 18, 2021 08:34
@Danil-Grigorev Danil-Grigorev requested a review from elmiko May 18, 2021 08:35
JoelSpeed
JoelSpeed reviewed May 18, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

This looks good, good separation of concerns into the different areas.

In general it would be good to see comments on public structs and public methods so that we can have a godoc style reference to what is going on here.

Also, I would like to see unit tests for the provider packages so that we know from the unit tests that they don't panic on init.

Comment thread pkg/cloud/cloud.go Outdated
Comment thread pkg/cloud/common/sources.go Outdated
@Danil-Grigorev Danil-Grigorev force-pushed the refactor-config-and-substitution branch 2 times, most recently from df03426 to b150e93 Compare May 18, 2021 14:11
Separate config, substitution and cloud package from controllers
9f154bc 
- Refactor tests to use assert and ensure full test coverage
@Danil-Grigorev Danil-Grigorev force-pushed the refactor-config-and-substitution branch from b150e93 to 9f154bc Compare May 18, 2021 14:25
@Danil-Grigorev Danil-Grigorev requested a review from JoelSpeed May 18, 2021 14:25
elmiko
elmiko approved these changes May 18, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

thanks Danil!
/lgtm

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 18, 2021
@lobziik
Copy link
Copy Markdown
Contributor

lobziik commented May 18, 2021

/lgtm

@openshift-bot
Copy link
Copy Markdown

openshift-bot commented May 18, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Copy Markdown

openshift-bot commented May 18, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 22a524b into openshift:master May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fedosin Fedosin Awaiting requested review from Fedosin

@lobziik lobziik Awaiting requested review from lobziik

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

1 more reviewer

@elmiko elmiko elmiko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@elmiko elmiko

@lobziik lobziik

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Danil-Grigorev @lobziik @openshift-bot @elmiko @JoelSpeed @openshift-merge-robot