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.

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

add support for multiple regions #21065

Merged
merged 4 commits into from
Jan 11, 2021

Conversation

ravinaik1312
Copy link
Contributor

What does this PR do?

This PR adds support for deploying functionbeat in multiple regions.

Why is it important?

Teams can deploy the functionbeat in multiple regions and

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 12, 2020
@@ -16,7 +16,7 @@ import (
)

var (
functionPattern = "^[A-Za-z][A-Za-z0-9\\-]{0,139}$"
functionPattern = "^[A-Za-z][A-Za-z0-9\\-]{0,29}$"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added this change because, as we're appending the function name to the role name, the role name would exceed AWS's limit of 64 characters. I can change the implementation based on the feedback from the maintainers. Other alternative is to add a conditional for creating the role.

Either ways, the 140 character limit for the function name needs to be reduced as we're already appending the function name in the role name.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good point. However, I am bit worried if we change the role name, we might forgot to change this number. Do you mind refactoring it so the length of the pattern depends on the role name prefix? Or at least add a comment to the code about why the max length is 29?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was off by one on the limit, it can be 30 instead of 29. I've updated the regex, tests and added a comment on how I got to the number 30.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 12, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: kvch commented: jenkins run tests

  • Start Time: 2021-01-11T15:27:43.603+0000

  • Duration: 24 min 44 sec

Test stats 🧪

Test Results
Failed 0
Passed 314
Skipped 4
Total 318

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 314
Skipped 4
Total 318

@ravinaik1312 ravinaik1312 force-pushed the beats-16935-support-multiple-region branch from 2db857c to 3024ba7 Compare September 12, 2020 02:28
@ycombinator ycombinator added Functionbeat Team:Integrations Label for the Integrations team labels Sep 12, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 12, 2020
@andresrc andresrc added the Team:Services (Deprecated) Label for the former Integrations-Services team label Sep 14, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-services (Team:Services)

@ph ph self-assigned this Dec 8, 2020
@ph ph self-requested a review December 8, 2020 13:39
@ravinaik1312
Copy link
Contributor Author

@ph thank you for self-requesting review, let me know if the changes makes sense, happy to discuss alternatives too!

@ph ph requested a review from kvch January 4, 2021 17:45
@ph
Copy link
Contributor

ph commented Jan 4, 2021

@kvch can you give a second pair of eye on this. I think this looks ok, I am a bit worried about the new limit and the impact on existing deployment?

@kvch
Copy link
Contributor

kvch commented Jan 11, 2021

jenkins run tests

@kvch
Copy link
Contributor

kvch commented Jan 11, 2021

Thank you!

@kvch kvch merged commit ee95f3d into elastic:master Jan 11, 2021
@kvch kvch added the needs_backport PR is waiting to be backported to other branches. label Jan 11, 2021
kvch pushed a commit to kvch/beats that referenced this pull request Jan 11, 2021
@kvch kvch added v7.12.0 and removed needs_backport PR is waiting to be backported to other branches. labels Jan 11, 2021
@ravinaik1312 ravinaik1312 deleted the beats-16935-support-multiple-region branch January 11, 2021 16:56
kvch added a commit that referenced this pull request Jan 13, 2021
(cherry picked from commit ee95f3d)

Co-authored-by: Ravi Naik <rnaik@godaddy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Functionbeat Team:Integrations Label for the Integrations team Team:Services (Deprecated) Label for the former Integrations-Services team v7.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Functionbeat][Lambda] Support multiple regions
7 participants