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.

Sign up for GitHub

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

Jump to bottom

operator: Refactor AWS and Azure allocators #10758

Merged
merged 3 commits into from
Apr 3, 2020
Merged

operator: Refactor AWS and Azure allocators #10758

merged 3 commits into from
Apr 3, 2020

Conversation

errordeveloper
Copy link
Contributor

@errordeveloper errordeveloper commented Mar 30, 2020 •
edited
Loading

This is the first step towards #9920.

$ tags=("operator_aws,operator_azure" "operator_aws" "operator_azure" "")
$ for t in "${tags[@]}" ; do (echo "building with -tags ${t}..." ; go build -tags "${t}" && du -hs operator) ; done
building with -tags operator_aws,operator_azure...
 83M    operator
building with -tags operator_aws...
 80M    operator
building with -tags operator_azure...
 60M    operator
building with -tags ...
 58M    operator
$ for t in "${tags[@]}" ; do (echo "[stripped] building with -tags ${t}..." ; go build -tags "${t}" -ldflags '-s -w' && du -hs operator) ; done
[stripped] building with -tags operator_aws,operator_azure...
 63M    operator
[stripped] building with -tags operator_aws...
 61M    operator
[stripped] building with -tags operator_azure...
 50M    operator
[stripped] building with -tags ...
 48M    operator
$

@errordeveloper errordeveloper added the release-note/misc This PR makes changes that have no direct user impact. label Mar 30, 2020
@errordeveloper errordeveloper requested a review from a team as a code owner March 30, 2020 13:25
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch 2 times, most recently from 597f243 to 8e74d32 Compare March 30, 2020 13:30
@errordeveloper
Copy link
Contributor Author

test-me-please

@coveralls
Copy link

coveralls commented Mar 30, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.5%) to 45.931% when pulling 197f6ef on pr/errordeveloper/operator-tags into b9d2105 on master.

aanm
aanm requested changes Mar 30, 2020
View reviewed changes
pkg/ipam/node_manager.go Outdated Show resolved Hide resolved
operator/provider_azure.go Outdated Show resolved Hide resolved
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch from 8e74d32 to 883032c Compare March 30, 2020 15:18
@errordeveloper errordeveloper requested a review from aanm March 30, 2020 15:19
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch from 883032c to f85ed0c Compare March 30, 2020 15:23
aanm
aanm reviewed Mar 30, 2020
View reviewed changes
operator/provider_aws.go Show resolved Hide resolved
operator/tasks/allocator_aws/allocator_aws.go Outdated Show resolved Hide resolved
operator/tasks/allocator_azure/allocator_azure.go Outdated Show resolved Hide resolved
tklauser
tklauser requested changes Apr 1, 2020
View reviewed changes
operator/main.go Outdated Show resolved Hide resolved
operator/main.go Outdated Show resolved Hide resolved
operator/tasks/allocator_aws/allocator_aws.go Outdated Show resolved Hide resolved
operator/tasks/allocator_azure/allocator_azure.go Outdated Show resolved Hide resolved
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch 2 times, most recently from a67e8a0 to 30575e5 Compare April 1, 2020 14:36
@errordeveloper errordeveloper requested review from tklauser and aanm April 1, 2020 14:36
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch from 30575e5 to e6a5ac5 Compare April 1, 2020 14:41
@errordeveloper

This comment has been minimized.

Sign in to view
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch 2 times, most recently from 8a0c742 to 9c350a7 Compare April 2, 2020 10:22
@errordeveloper

This comment has been minimized.

Sign in to view
tklauser
tklauser requested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Two small nits, otherwise LGTM.

pkg/ipam/allocator/provider.go

// AllocatorProvider defines the functions of IPAM provider front-end
// these are implemented by e.g. pkg/ipam/allocator/{aws,azure}.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: drop this empty line so the godoc comment gets associated with type AllocatorProvider.

Copy link
Member

Choose a reason for hiding this comment

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

This is still unresolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure?

pkg/ipam/allocator/azure/azure.go Outdated Show resolved Hide resolved
@errordeveloper
Copy link
Contributor Author

@tklauser thanks! I'm not sure how this happen, I guess I'm not paying all the attention to IDE warnings as macOS build is partly broken and there are far too many warnings.

@errordeveloper
Copy link
Contributor Author

Ah, actually I completely forgot to add godoc comments to new functions, that's what happen :)

@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch from 9c350a7 to 94526d3 Compare April 2, 2020 11:00
@errordeveloper

This comment has been minimized.

Sign in to view
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch from 94526d3 to a4eab69 Compare April 2, 2020 14:36
@errordeveloper
Copy link
Contributor Author 

17:17:19  [Fail] K8sServicesTest Checks ClusterIP Connectivity [AfterEach] Checks service on same node

...will re-run

@errordeveloper
operator: Create separate metrics package
61930c7 
Signed-off-by: Ilya Dmitrichenko <[email protected]>
@errordeveloper
ipam: Export and rename the getter-updater interface
854c979 
Signed-off-by: Ilya Dmitrichenko <[email protected]>
@errordeveloper
operator: Refactor AWS & Azure allocators
197f6ef 
- use build tags to control inclusion of SDK dependencies
- include both providers by default to keep packaging as is

Signed-off-by: Ilya Dmitrichenko <[email protected]>
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch from a4eab69 to 197f6ef Compare April 2, 2020 17:58
@errordeveloper
Copy link
Contributor Author

test-me-please

@errordeveloper errordeveloper merged commit d3f83ac into master Apr 3, 2020
@errordeveloper errordeveloper deleted the pr/errordeveloper/operator-tags branch April 3, 2020 09:40
@errordeveloper errordeveloper mentioned this pull request Apr 3, 2020
Closed
@errordeveloper errordeveloper mentioned this pull request Apr 14, 2020
Merged
@errordeveloper
Copy link
Contributor Author

I just got a little puzzled by number above, and what I didn't realise is that in my test I wasn't stripping the binaries. I've updated the description for the record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

@joestringer joestringer joestringer left review comments

@tklauser tklauser tklauser approved these changes

@tgraf tgraf Awaiting requested review from tgraf

Assignees
No one assigned
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@errordeveloper @coveralls @tklauser @joestringer @aanm