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

feat: TrafficRouting support with AWS App Mesh (#1401) #1606

Merged
merged 2 commits into from
Feb 3, 2022
Merged

feat: TrafficRouting support with AWS App Mesh (#1401) #1606

merged 2 commits into from
Feb 3, 2022

Conversation

kiranmeduri
Copy link
Contributor

@kiranmeduri kiranmeduri commented Oct 21, 2021 •
edited
Loading

TrafficRouting support with AWS App Mesh

  • Supports canary based strategy with weighted traffic splitting using App Mesh
  • Added getting-started doc for App Mesh

#1401

Organization: Amazon Web Services

Signed-off-by: Kiran Meduri meduri@amazon.com

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@kiranmeduri kiranmeduri force-pushed the appmesh branch 2 times, most recently from 6ca37e6 to b014499 Compare October 21, 2021 20:42
jessesuen
jessesuen reviewed Oct 22, 2021
View reviewed changes
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

Did a quick take on API semantics but will take a deeper look later. Thanks!

pkg/apis/rollouts/v1alpha1/types.go Outdated Show resolved Hide resolved
docs/getting-started/appmesh/index.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 22, 2021 •
edited
Loading

Codecov Report

Merging #1606 (1e5df6e) into master (f388937) will increase coverage by 0.01%.
The diff coverage is 82.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
+ Coverage   82.33%   82.35%   +0.01%     
==========================================
  Files         116      118       +2     
  Lines       16393    16785     +392     
==========================================
+ Hits        13497    13823     +326     
- Misses       2217     2272      +55     
- Partials      679      690      +11
Impacted Files Coverage Δ
rollout/trafficrouting/appmesh/appmesh.go 77.51% <77.51%> (ø)
rollout/trafficrouting/appmesh/resource_client.go 78.57% <78.57%> (ø)
rollout/controller.go 76.49% <93.93%> (+1.10%) ⬆️
controller/metrics/rollouts.go 77.90% <100.00%> (+0.79%) ⬆️
.../apis/rollouts/validation/validation_references.go 87.26% <100.00%> (+2.06%) ⬆️
rollout/trafficrouting.go 81.05% <100.00%> (+0.60%) ⬆️
utils/defaults/defaults.go 88.35% <100.00%> (+0.32%) ⬆️
rollout/trafficrouting/istio/controller.go 52.43% <0.00%> (+1.62%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f388937...1e5df6e. Read the comment docs.

@kiranmeduri kiranmeduri force-pushed the appmesh branch 5 times, most recently from a60f7f3 to 3f14c46 Compare October 29, 2021 16:33
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

No Coverage information No Coverage information
5.8% 5.8% Duplication

@kiranmeduri kiranmeduri requested a review from jessesuen November 1, 2021 16:22
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
5.4% 5.4% Duplication

@jessesuen
Copy link
Member

@kiranmeduri sorry for not looking at this sooner. I'd like to start reviewing this for potential inclusion in the upcoming 1.2 release. Is this ready for review? Some of the tests/linting are failing

@kiranmeduri
Copy link
Contributor Author

@jessesuen thanks for update, let me sync again and check the failures. I will update the PR and notify.

@kiranmeduri kiranmeduri force-pushed the appmesh branch 2 times, most recently from a76e812 to d3021bc Compare January 14, 2022 22:06
jessesuen
jessesuen reviewed Jan 14, 2022
View reviewed changes
utils/appmesh/appmesh.go Outdated Show resolved Hide resolved
@kiranmeduri kiranmeduri force-pushed the appmesh branch 2 times, most recently from b5e2b6c to fd4351d Compare January 15, 2022 00:06
@kiranmeduri kiranmeduri marked this pull request as draft January 15, 2022 00:07
@kiranmeduri kiranmeduri force-pushed the appmesh branch 2 times, most recently from 61df6c1 to 596cbf3 Compare January 21, 2022 21:38
@kiranmeduri
Copy link
Contributor Author

@jessesuen PR is ready for review but unclear how to fix my "verify codegen" check that is failing. Can you point me in right direction? Thanks

@kiranmeduri kiranmeduri marked this pull request as ready for review January 21, 2022 22:16
@kiranmeduri kiranmeduri force-pushed the appmesh branch 3 times, most recently from 37da53c to bfe0261 Compare January 23, 2022 19:06
@harikrongali harikrongali self-requested a review January 26, 2022 22:22
@jessesuen
Copy link
Member

@jessesuen PR is ready for review but unclear how to fix my "verify codegen" check that is failing. Can you point me in right direction? Thanks

Did you try running make codegen target? That should fix it.

@kiranmeduri
Copy link
Contributor Author

@jessesuen PR is ready for review but unclear how to fix my "verify codegen" check that is failing. Can you point me in right direction? Thanks

Did you try running make codegen target? That should fix it.

Yes i did and make codegen succeeds. However, I notice that it generates new versions of manifest file as my controller-gen is 0.5.0 instead of upstream 0.7.0. Do you think that can be the culprit? In the meantime I will update that and see.

Thanks

@kiranmeduri kiranmeduri force-pushed the appmesh branch 3 times, most recently from 4a76a40 to ed25122 Compare January 31, 2022 23:00
@kiranmeduri
Copy link
Contributor Author

@jessesuen PR is ready for review but unclear how to fix my "verify codegen" check that is failing. Can you point me in right direction? Thanks

Did you try running make codegen target? That should fix it.

Yes i did and make codegen succeeds. However, I notice that it generates new versions of manifest file as my controller-gen is 0.5.0 instead of upstream 0.7.0. Do you think that can be the culprit? In the meantime I will update that and see.

Thanks

Figured this out, make codegen is generating files in $GOPATH/src while my project path is not on GOPATH. May be there is a way to make it work properly. Thanks anyways

@kiranmeduri kiranmeduri force-pushed the appmesh branch 2 times, most recently from 15f6c73 to e9c3c67 Compare February 1, 2022 18:55
@kiranmeduri
Copy link
Contributor Author

@jessesuen this PR is ready for review. Please take a look and I would really appreciate if we can get it into next release (1.2). Thanks

@harikrongali
Copy link
Contributor

@kiranmeduri few nit picks other than that lgtm.
can you please rebase to master

@kiranmeduri
feat: TrafficRouting support with AWS App Mesh
bc3c36a 
- Supports canary based strategy with weighted traffic splitting
- Added getting-started doc for App Mesh

argoproj#1401

Signed-off-by: Kiran Meduri <meduri@amazon.com>
@kiranmeduri
Added a comment about int64 conversion in appmesh.go
1e5df6e 
Signed-off-by: Kiran Meduri <meduri@amazon.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
4.3% 4.3% Duplication

jessesuen
jessesuen approved these changes Feb 3, 2022
View reviewed changes
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

Great work!

@jessesuen jessesuen merged commit 5f0f8b4 into argoproj:master Feb 3, 2022
@jessesuen jessesuen mentioned this pull request Feb 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harikrongali harikrongali harikrongali approved these changes

@jessesuen jessesuen jessesuen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kiranmeduri @jessesuen @harikrongali