Skip to content

Conversation

@erikfuller
Copy link
Contributor

What type of PR is this?
cleanup

Which issue does this PR fix:
This PR includes basic readability improvements, including refactoring of large methods, more descriptive naming for variables and functions, and consistency in imports. There should be NO FUNCTIONAL CHANGES in this PR.

One thing I would like to get feedback on are the contents of the Developer Cheat Sheet.

What does this PR do / Why do we need it:
This is part of a larger refactor which will ultimately remove the lattice data store, specifically for target groups.

Testing done on this change:

└─[$] <git:(refactor-1*)> [laptop] make presubmit
pkg/deploy/lattice/listener_synthesizer.go
New file modification detected in the Git working tree. Please check in before commit.
  - pkg/deploy/lattice/listener_synthesizer.go
?   	github.com/aws/aws-application-networking-k8s/pkg/apis/applicationnetworking/v1alpha1	[no test files]
ok  	github.com/aws/aws-application-networking-k8s/pkg/aws	0.585s	coverage: 24.2% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/aws/services	1.121s	coverage: 9.7% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/config	1.052s	coverage: 32.9% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/deploy	2.609s	coverage: 19.4% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/deploy/externaldns	1.613s	coverage: 75.6% of statements
?   	github.com/aws/aws-application-networking-k8s/pkg/k8s	[no test files]
?   	github.com/aws/aws-application-networking-k8s/pkg/model/lattice	[no test files]
?   	github.com/aws/aws-application-networking-k8s/pkg/runtime	[no test files]
?   	github.com/aws/aws-application-networking-k8s/pkg/utils	[no test files]
?   	github.com/aws/aws-application-networking-k8s/pkg/utils/gwlog	[no test files]
?   	github.com/aws/aws-application-networking-k8s/pkg/utils/retry	[no test files]
?   	github.com/aws/aws-application-networking-k8s/pkg/utils/ttime	[no test files]
ok  	github.com/aws/aws-application-networking-k8s/pkg/deploy/lattice	3.399s	coverage: 85.0% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/gateway	5.200s	coverage: 77.8% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/latticestore	5.289s	coverage: 64.1% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/model/core	6.379s	coverage: 61.7% of statements
ok  	github.com/aws/aws-application-networking-k8s/pkg/model/core/graph	6.412s	coverage: 17.2% of statements
ok  	github.com/aws/aws-application-networking-k8s/controllers	1.406s	coverage: 0.0% of statements
ok  	github.com/aws/aws-application-networking-k8s/controllers/eventhandlers	2.106s	coverage: 53.5% of statements

Automation added to e2e:
No changes

Will this PR introduce any new dependencies?:
No

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
n/a

Does this PR introduce any user-facing change?:
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -0,0 +1,61 @@
# Developer Cheat Sheet
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please make sure you agree with what's in here

Copy link
Contributor

Choose a reason for hiding this comment

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

nice, an easy read

Copy link
Contributor

@mikhail-aws mikhail-aws left a comment

Choose a reason for hiding this comment

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

Not sure that all the changes have to be in same PR, but looks ok.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6397137956

  • 281 of 599 (46.91%) changed or added relevant lines in 29 files are covered.
  • 14 unchanged lines in 9 files lost coverage.
  • Overall coverage decreased (-0.01%) to 46.013%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/deploy/lattice/targets_synthesizer.go 1 2 50.0%
pkg/gateway/model_build_listener.go 8 9 88.89%
pkg/deploy/lattice/service_network_synthesizer.go 4 6 66.67%
pkg/gateway/model_build_targets.go 5 7 71.43%
pkg/model/core/route.go 0 2 0.0%
pkg/deploy/lattice/listener_manager.go 4 7 57.14%
pkg/deploy/lattice/service_network_manager.go 18 21 85.71%
pkg/deploy/lattice/rule_manager.go 15 19 78.95%
pkg/deploy/lattice/target_group_manager.go 26 31 83.87%
pkg/deploy/stack_deployer.go 0 5 0.0%
Files with Coverage Reduction New Missed Lines %
controllers/gatewayclass_controller.go 1 0.0%
controllers/gateway_controller.go 1 0.0%
controllers/route_controller.go 1 0.0%
pkg/gateway/model_build_listener.go 1 74.73%
pkg/gateway/model_build_rule.go 1 82.82%
controllers/pod_controller.go 2 0.0%
controllers/service_controller.go 2 0.0%
controllers/serviceexport_controller.go 2 0.0%
controllers/serviceimport_controller.go 3 0.0%
Totals Coverage Status
Change from base Build 6355409430: -0.01%
Covered Lines: 3855
Relevant Lines: 8378

💛 - Coveralls

@erikfuller erikfuller merged commit 17dc47e into aws:main Oct 3, 2023
@erikfuller erikfuller deleted the refactor-1 branch October 3, 2023 18:53
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.

3 participants